TG-800 Add API support #220
Conversation
niccolomineo
force-pushed
the
feature/162-openapi
branch
from
9debed0 to
61365f4
Compare
niccolomineo
force-pushed
the
feature/162-openapi
branch
from
61365f4 to
3e9cf27
Compare
niccolomineo
requested review from
daniele-20tab,
filippo-20tab,
gabbork,
mmazzocchetti,
pauloxnet,
rafleze,
trottomv and
valentinogagliardi
niccolomineo
force-pushed
the
feature/162-openapi
branch
2 times, most recently
from
3bcfff3 to
cd87a14
Compare
niccolomineo
force-pushed
the
feature/162-openapi
branch
from
cd87a14 to
48a3dbb
Compare
| "djangorestframework_camel_case.parser.CamelCaseMultiPartParser", | ||
| "djangorestframework_camel_case.parser.CamelCaseJSONParser", | ||
| ], | ||
| "DEFAULT_PERMISSION_CLASSES": ["rest_framework.permissions.IsAuthenticated"], |
There was a problem hiding this comment.
I am unsure whether to use AllowAny here. What do you think?
niccolomineo
force-pushed
the
feature/162-openapi
branch
3 times, most recently
from
5ca2b96 to
c6b1829
Compare
| -r base.in | ||
| djangorestframework-camel-case~=1.3.0 | ||
| djangorestframework~=3.13.0 | ||
| drf-spectacular~=0.22.0 |
There was a problem hiding this comment.
Let's discuss what exactly we should ask the user for, with the aim to make DRF an optional dependency.
niccolomineo
force-pushed
the
feature/162-openapi
branch
from
c6b1829 to
d896d3d
Compare
niccolomineo
force-pushed
the
feature/162-openapi
branch
2 times, most recently
from
813ae10 to
606b7c8
Compare
| [ | ||
| path( | ||
| "schema/", | ||
| staff_member_required( |
There was a problem hiding this comment.
instead using staff_member_required here, drf_spectacular has a generic settings to protect his endpoints:
SPECTACULAR_SETTINGS = {
"SERVE_PERMISSIONS": ["rest_framework.permissions.IsAuthenticated"],`
...
}
| ] | ||
|
|
||
| MIDDLEWARE = [ | ||
| "corsheaders.middleware.CorsMiddleware", |
There was a problem hiding this comment.
is this strictly related to API documentation feature?
I would suggest doing a separate PR
There was a problem hiding this comment.
This PR has evolved in its scope, hence the new title.
| EMAIL_URL = var.email_url | ||
| SENTRY_DSN = var.sentry_dsn | ||
| DJANGO_SECRET_KEY = random_password.django_secret_key.result | ||
| DJANGO_ENABLE_API_DOC = var.environment_slug == "prod" ? "False" : "True" |
There was a problem hiding this comment.
I leave a generic comment in this point as "food for thought" on what is the best way to integrate this functionality into the template (Talos), which I think is a great starting point.
But I'm not so sure that integrating it by default in the template could be the best solution, perhaps the most suitable solution could be a public package on a separate repository that can be integrated into the Talos Django backend with a simple pip install and adding it to the INSTALLED_APPS (?)
There was a problem hiding this comment.
The reason we're embedding the API doc by default is that, this way, the API can be inspected and tested by our QA testers, as well.
niccolomineo
force-pushed
the
feature/162-openapi
branch
from
606b7c8 to
d569ca4
Compare
| "djangorestframework_camel_case.render.CamelCaseJSONRenderer", | ||
| ], | ||
| "DEFAULT_SCHEMA_CLASS": "drf_spectacular.openapi.AutoSchema", | ||
| } |
There was a problem hiding this comment.
Add also this settings
"JSON_UNDERSCOREIZE": {"no_underscore_before_number": True},
By default djangorestframework-camel-case leave an underscore before numbers (ex. customerAddress_1 and customerAddress_2) and this often leads to unwanted effects like in this issue:
vbabiy/djangorestframework-camel-case#50 (comment)
|
Reopen when the US will be added in a sprint |
Closes #84