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

[WiP] Simplified REST structures #1990

Closed
wants to merge 27 commits into from
Closed

Conversation

tobyS
Copy link
Contributor

@tobyS tobyS commented May 16, 2017

This PR deals to discuss simplified REST structures, based on the initial approach presented here.

The basic idea is to ease usage of the REST API, especially for JSON based clients.

  • Specify initial draft for discussion
  • Discuss and evolve approaches
  • Add specification for linking alias URIs in types (if accepted)
  • Provide prototype implementation
    • Implement mechanism to resolve resources from alias URLs
    • Implement custom parsing approach for simplified content create
    • Adjust resource resolver approach to work on URL parameters since symfony request matching is not how Core implementation works
    • Implement functional test for simplified content create
    • Re-check specs for re-worked alias URI updates

Further TODOs:

  • Introduce tests for all alias URIs with non existent identifiers (should return 404)

tobyS added 2 commits May 15, 2017 15:49
This includes several ideas:

1. In order to ease usage of the REST API for JavaScript but also PHP
   consumers, simplified media type definitions can be created.
2. Simplified media types are used right beside the original ones which
   might be valid either infinitely or which might be replaced
   graduately by the simplified versions (evolution).
3. In the first step, simplified media types are used only to make
   create/update operations easier in that the data structures they
   define are easier to create and work with.
4. Also, simplified media types require less information to be present
   and will be automatically completed with sane default values by the
   server.

In addition, this commit already contains hints on the new concept of
"alias URIs" (see next commits in this branch).
Alias URIs permit more user friendly IDs/URLs while maintaining a
consistent state throughout caches.
@tobyS
Copy link
Contributor Author

tobyS commented May 16, 2017

Feedback on simplified ContentCreate in REST API

These are the initial review comments and braindump about the simplified REST structure.

Review based on: https://github.com/kamilmusial/EzSystemsRestv3BundlePrototype/blob/46b8bef69e077d76dc7deaf589400dd491f8cfc0/doc/specifications/drafts/rest_content_managemet.md

General

  • The general idea is very good
  • Simplified structures to access the REST API make sense on top of existing, very extensive API structures
  • Simplified access should be realized through additional media types being provided (e.g. "application/vnd.ez.api.ContentCreate+json; version=2.0" or "application/vnd.ez.api.SimplifiedContentCreate+json")
  • The current state of REST specification (& documentation?) is rather complicated, it is recommended to research a better solution self-documentation of the REST API that can take away a big portion of the spec, too (TS can take that over, if desired)

Complex ContentCreate-Reques

    {
        "ContentCreate": {
            "ContentType": "article",
  • Reference should always be URIs
  • Since ID based reference URIs seem to be a pain point, a potential solution is discussed in Alias URLs (see below)
  • This also applies to _parentUrlAliasPath, ContentSection, Owner, ... references
    "LocationCreate": {
        "_parentUrlAliasPath": "/places-tastes/tastes/"
    },
    "ContentSection": 1,
    "Owner": "jessica",
    "alwaysAvailable": "false",
    "modificationDate": "2017-03-02T12:00:00",
    "remoteId": "remote123456",
    "fields": {
        "title": "This is a title",
  • What about different language here? Does this walk hand in hand with default languages etc. in the API?
  • Otherwise we should add an additional level of nesting here which would also allow speficying multiple languages for a content at once:
        "fields": {
            "en-US": {
                "title": "This is a title",
                ...
            }
        }

Proposal: URL Aliasing

  • People insist on using alias identifiers (e.g. ContentType identifier, user name, etc.) instead of ID URLs for accessing and referencing entities
  • Still, avoiding duplication of entity state being available at different URIs should be key (e.g. for caching reasons)
  • Also, referencing should always use a URI and never only the identifier itself
  • Idea: Provide alias URLs for common identifier based access and return "308" (or maybe 303?) status with the canonical URL, e.g.:
    GET /content/types/_by_identifier/article
    HTTP/1.1 308 Permanent Redirect
    Location: /content/types/23
  • URL aliases can be used for referencing during creation and will be resolved by the server automatically
  • Alias URLs can be returned when fetching the canonical content, too, e.g.:
        <ContentType href="/content/types/32" media-type="application/vnd.ez.api.ContentType+xml">
          <id>32</id>
          <status>DRAFT</status>
          <identifier>newContentType</identifier>
          <-- ... -->

          <_uris>
            <link href="/content/types/32" rel="canonical" />
            <link href="/content/types/_by_identifier/newContentType" rel="self" />
            <!-- ... potentially more reference links ... -->
          </_uris>
        </ContentType>
  • This provides self-documenting responses that allow developers to discover these alias URIs
  • It is unclear what rel is best for non-canonical URIs (canonical is set by IANA). Possibilities are:
    • rel="self" which generally denotes a URL to the very same document in e.g. HTML. This conforms to at least "some" standard, but might not be very clear for the user at first
    • custom relation. e.g. rel="//ez.no/api/rest/relation/alias-uri" this allows the developer to easily grasp the meaning of the relation and allows to have the documentation right at an easily discoverable place

"title": "This is a title",
"intro": {
"xhtml5edit": "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<section xmlns=\"http://ez.no/namespaces/ezpublish5/xhtml5/edit\"><p>Article intro.</p></section>\n",
"xml": "<?xml version=\"1.0\" encoding=\"UTF-8\"?>\n<section xmlns=\"http://docbook.org/ns/docbook\" xmlns:xlink=\"http://www.w3.org/1999/xlink\" xmlns:ezxhtml=\"http://ez.no/xmlns/ezpublish/docbook/xhtml\" xmlns:ezcustom=\"http://ez.no/xmlns/ezpublish/docbook/custom\" version=\"5.0-variant ezpublish-1.0\"><para>Article intro.</para></section>\n"
Copy link
Member

Choose a reason for hiding this comment

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

@tobyS do you imply by this, that both xhtml5edit and xml values are required or just one of them depending on API consumer needs? I'm asking because if we want to simplify, only one should be required, but both supported (just IMHO).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alongosz The latter one. The attributes set for a field are only determined by the field type.

@andrerom andrerom mentioned this pull request May 23, 2017
3 tasks

A request/response cycle for an alias URI is supposed to be executed as follows:

:Resource: /content/types/_by_identifier/article
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why you aim to duplicate the existing mechanism for this?
Referring here to ?remoteId, ?identifier, ?locationPath, ?roleId, ?email and ?login, I would have expected the concepts here to extend that for the identified scenarios.

For instance the example here can already be expressed using:

<contentTypeByIdentifier media-type="" href="/api/ezp/v2/content/types{?identifier}"/>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good that you pointed me to this one. Did not see it, yet. I'll dig into how this is realized, because spec is not complete here (missing media types, etc.).

In general, ?something=anything is a filter on a list, so that might return 0, 1 or even more results, which does not make that a reliable source for referencing a unique item in a link.

But, let's see how it is implemented anyway …

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, the behavior is returning a list, but also throwing 404 if the list is empty. This is not entirely correct from a REST PoV but not worth changing. So, agreed with @andrerom, we should re-use this mechanism for the alias URLs. I'll update the spec and also add some more info on the media types returned by such requests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Research on how this is specified/implemented in detail revealed that there is no consistent way, yet: https://gist.github.com/tobyS/c1004831f87291e19662c28a0edef2f2 I recommend switching to redirects everywhere since this seems to be most common and gives only a tiny BC break. With that, we can implement the remaining alias URLs in the same way and allow them to be used by the simplified creates. I'll update the specs and code accordingly.

Copy link
Contributor

@andrerom andrerom Jun 13, 2017

Choose a reason for hiding this comment

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

I think I had redirection in mind here, so just adjusting the semantic to existing examples where redirect is used. And in parallel we can consider fixing existing cases to make api more consistent.

( /cc @bdunogier What do you think? )

Only concern I have with redirect is cache, can we somehow cache them, shortly lived, and even tag them to make sure we purge it on updates? Will Vanish for instance allow that? anyway, that is separate topic probably.

@ezsystems ezsystems deleted a comment from ezrobot Jun 13, 2017
@ezsystems ezsystems deleted a comment from ezrobot Jun 19, 2017
This will be used in parsing simplified content create to resolve the
referred resources. Pimping this a little bit gives potential to
simplify quite a bit of controller code in REST server, too.
@ezsystems ezsystems deleted a comment from ezrobot Jun 20, 2017
@tobyS
Copy link
Contributor Author

tobyS commented Jun 21, 2017

Current state overview

  • Central concept are alias URIs to make it easier to refer to resources in simplified creates
    • Controllers support alias URIs in the form of /list?uniqueIdentifier=something
    • The new ResourceResolver class should deal as the central entry point for resolving alias URIs
    • This can also be used to simplify many controllers later
    • Currently, the ResourceResolver is implemented on basis of the RequestParsers found in Core/, but these are not used anymore in Bundle/
    • The RequestParser in Bundle/ does not support fine-grained "type" (aka route) names to identify which resource is to be fetched by which criteria, therefore the ResourceResolver needs to be updated to load resources correctly anyway (maybe match on the Request object itself?)
  • Parsing of simplified content create is done inside the controller right now
    • This is fine as a first step, since we have no other exceptions from the standard behavior
    • If the concept of simplified structure support turns out to be good, we can refactor this handling into dedicated, more re-usable services
  • A functional test for the simplified content create is missing

.. code:: http

HTTP/1.1 307 Temporary Redirect
Location: /content/types/23
Copy link
Contributor

@andrerom andrerom Jun 22, 2017

Choose a reason for hiding this comment

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

Note to self: we can cache these with Varnish: http://book.varnish-software.com/4.0/chapters/VCL_Basics.html#the-initial-value-of-beresp-ttl

Assuming we also return cache headers mentioned there, and proper tag header so ezplatform-http-cache can invalidate it (needed if we cache this).

=============================================================== ===============================
Alias URI Target
--------------------------------------------------------------- -------------------------------
/content/objects/_by_remote_id/<remote-ID> /content/objects/<ID>
Copy link
Contributor

Choose a reason for hiding this comment

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

Weren't this supposed to use same form as /content/types/?identifier=article referred to above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, missed to update this. Needs to be fixed.

@ezsystems ezsystems deleted a comment from ezrobot Jun 22, 2017
@ezsystems ezsystems deleted a comment from ezrobot Jun 22, 2017
@ezsystems ezsystems deleted a comment from ezrobot Jun 22, 2017
@ezsystems ezsystems deleted a comment from ezrobot Jun 22, 2017
@ezsystems ezsystems deleted a comment from ezrobot Jun 22, 2017
@alongosz alongosz closed this May 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants