Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Schemas & client libraries. #4179

Merged
merged 32 commits into from Jul 4, 2016
Merged

Schemas & client libraries. #4179

merged 32 commits into from Jul 4, 2016

Conversation

tomchristie
Copy link
Member

@tomchristie tomchristie commented Jun 8, 2016

Support for schema generation & dynamic client libraries.

See the new tutorial section, schema documentation, and api client documentation.

Needed to complete tutorial 7:

  • Graceful behavior with or without Core API installed.
  • Add --auth basic support to Core API.
  • Check permissions on schema generation.
  • Include pagination parameters in automatic schemas.

Other work as part of this, prior to 3.4.0 release.

  • Include filter parameters in automatic schemas.
  • Schema documentation.
  • Handle Document, Error and Link in JSON renderer?
  • Handle ListSerializer and other non-form.
  • Handle multipart vs json and/or other encodings.
  • Tests.
  • Handle text downloads.
  • Client library documentation.
  • Add coreapi to list of optional packages on homepage.

Moved to separate tickets:

  • Easier overrides on SchemaGenerator
  • Document using OpenAPI and other schema formats.
  • Move alternate formats in coreapi into separate packages. (HTML, OpenAPI, HyperSchema, HAL...)

Deferred:

  • Handle binary downloads. Not strictly required for 3.4
  • Use id not pk in schema representations or change serializer representations? Not strictly required for 3.4
  • Add to quickstart Not strictly required for 3.4

@tomchristie tomchristie added this to the 3.4.0 Release milestone Jun 8, 2016

def get(self, request, *args, **kwargs):
if request.accepted_renderer.format == 'corejson':
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason why this couldn't be brought into the CoreJSONRenderer?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is less what to do with CoreJSONRenderer and more about making sure that we preserve the behavior of that view for anything that isn't a schema renderer.

(Also I'd consider that part still slightly in flux - clearly needs refinement)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

making sure that we preserve the behavior of that view for anything that isn't a schema renderer

Right, my main concern with that line (which I'm sure will probably change another few times before this is merged) is that it's hard-coding corejson as the only renderer with schema support.

Also I'd consider that part still slightly in flux - clearly needs refinement

Definitely understandable.

@@ -790,3 +791,17 @@ def render(self, data, accepted_media_type=None, renderer_context=None):
"test case." % key
)
return encode_multipart(self.BOUNDARY, data)


class CoreJSONRenderer(BaseRenderer):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a move in 3.1 to remove features which had third-party dependencies from the core and put them into third-party packages that were still highly visible. See django-rest-framework-xml, django-rest-framework-yaml, and django-rest-framework-oauth for examples.

Is that still something we are pushing for?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kinda. I see coreapi as a foundational thing here, so it's a bit different.

The various types of schema and docs that you can use it to generate will be third party, yup.
So eg we can have third party packages for schema formats: Swagger / API Blueprint / JSON Hyperschema, for docs templates driven by a coreapi.Document object, and for various hypermedia styles.

Using coreapi ensures that we're able to provide a common interface for all of those, so I don't have any great issues with pulling it in. If it's in core, then we're making the promise that it's an interface that is available to third-party devs, which should help drive folks making schema renderers / hypermedia renderers and docs renderers. Having said that, we could push for it to be third-party, if we wanted to, so I might be open to discussion.


router = DefaultRouter(schema_title='Server Monitoring API')

The schema will be included in by the root URL, `/`, and presented to clients
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"included in by" seems confusing. Maybe take out the "by"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, that's a typo.

@tomchristie
Copy link
Member Author

Remaining issues have now been split out into individual tickets.
Plan is to prepare up a 3.4.0 release, then resolve as many remaining tickets in the time remaining before Thursday 14th July, with a release planned on or before that date.

(Ensure it will be released prior to DjangoCon US, and give at least one clear working day to resolve any ciritical issues, post-release)

@marcgibbons
Copy link
Contributor

💯

@tomchristie
Copy link
Member Author

Yeah. Indeed.

@@ -127,6 +130,17 @@ def to_html(self, request, queryset, view):
template = loader.get_template(self.template)
return template_render(template, context)

def get_fields(self, view):
filter_class = getattr(view, 'filter_class', None)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it make more sense to use self.get_filter_class here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly, tho we don't actually have a queryset/model available to us at this point, so couldn't dynamically generate the filter class if none was set explicitly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the problems with just calling view.get_queryset()?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that'd probably work okay.

rlucioni pushed a commit to openedx/course-discovery that referenced this pull request Jul 12, 2017
DRF 3.4.7 is the same version run by edx/credentials. Release notes are at http://www.django-rest-framework.org/topics/release-notes/#34x-series. Highlights include: support for Django 1.10 (encode/django-rest-framework#4158 - sigh), support for schema generation (encode/django-rest-framework#4179 - required for newer versions of django-rest-swagger), and a change that prevents paginated views from re-running queries when count queries return 0 (encode/django-rest-framework#4201 - this explains the expected query count changes made in tests).

LEARNER-1590
rlucioni pushed a commit to openedx/course-discovery that referenced this pull request Jul 12, 2017
DRF 3.4.7 is the same version run by edx/credentials. Release notes are at http://www.django-rest-framework.org/topics/release-notes/#34x-series.

Highlights include:
    - [Support for Django 1.10](encode/django-rest-framework#4158) - sigh
    - [Support for schema generation](encode/django-rest-framework#4179) - required for newer versions of django-rest-swagger
    - A [change](encode/django-rest-framework#4201) that prevents paginated views from re-running queries when count queries return 0 - this explains the expected query count changes made in tests

LEARNER-1590
rlucioni pushed a commit to openedx/course-discovery that referenced this pull request Jul 12, 2017
DRF 3.4.7 is the same version run by edx/credentials. Release notes are at http://www.django-rest-framework.org/topics/release-notes/#34x-series. Highlights include:

1. [Support for Django 1.10](encode/django-rest-framework#4158) - sigh
2. [Support for schema generation](encode/django-rest-framework#4179) - required for newer versions of django-rest-swagger
3. A change that [prevents paginated views from re-running queries](encode/django-rest-framework#4201) when count queries return 0 - this explains the expected query count changes made in tests

LEARNER-1590
rlucioni pushed a commit to openedx/course-discovery that referenced this pull request Jul 12, 2017
DRF 3.4.7 is the same version run by edx/credentials. Release notes are at http://www.django-rest-framework.org/topics/release-notes/#34x-series. Highlights include:

1. [Support for Django 1.10](encode/django-rest-framework#4158) - sigh
2. [Support for schema generation](encode/django-rest-framework#4179) - required for newer versions of django-rest-swagger
3. A change that [prevents paginated views from re-running queries](encode/django-rest-framework#4201) when count queries return 0 - this explains the expected query count changes made in tests

LEARNER-1590
@swehba
Copy link

swehba commented Nov 19, 2018

@tomchristie FYI, the links in your original comment at the top of this thread are broken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants