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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resource Versioning Profile #1333

Open
wants to merge 27 commits into
base: gh-pages
from

Conversation

Projects
None yet
4 participants
@gabesullice
Copy link
Contributor

gabesullice commented Dec 13, 2018

First! 馃榿 (well, sorta... @ethanresnick submitted one first, but does that really count?)

This is a profile extracted from Drupal's JSON:API implementation.

Drupal has long had support for moderated and revisioned content. We plan to support this in our project and we hope to land the feature quite soon鈥攚e're just cleaning up nitpicks and then it will land after we're out of a feature freeze (we're stabilizing a v2 right now).

We figured this would be a great way to pilot the new profile process.


I tried to make this profile very flexible while taking heavy inspiration from RFC 5829 - Link Relation Types for Simple Version Navigation between Web Resources.

This PR is still a work-in-progress and is not yet finalized, but I'm hoping to get a jump on any egregious oversights.

Before it is finalized, I would like for this profile to also include a set of recommended links and for it to include language about providing a version history endpoint. I don't think either of those things will be a requirement for adopting the profile though. Done!

/cc @wimleers @e0ipso

gabesullice added some commits Dec 12, 2018

@gabesullice

This comment has been minimized.

Copy link
Contributor

gabesullice commented Dec 13, 2018

@e0ipso

This comment has been minimized.

Copy link

e0ipso commented Dec 13, 2018

One thing that we tried (or at least I certainly kept that in mind) is the ability to negotiate versions based on flexible criteria. Different applications will have a slightly different concept of versioning. Some may want to negotiate a resource based on revision-id, others based on relative position to the published version, others based on a date time authoring, etc.

The proposed solution in the Drupal module will take all that into account. That may make this profile adequate for other JSON:API implementations, other than Drupal's. If other projects adopt this as well, I'm sure clients and tooling will follow.

I think that standard clients and common tooling is key for the success of the ecosystem, like GraphiLQ and Apollo have proven in the GraphQL ecosystem.

@wimleers

This comment has been minimized.

Copy link
Contributor

wimleers commented Dec 13, 2018

Can't wait to get feedback from @ethanresnick , @dgeb and others!

gabesullice added some commits Dec 13, 2018

@gabesullice

This comment has been minimized.

Copy link
Contributor

gabesullice commented Dec 20, 2018

I've just added sections for links and for a version history endpoint. IMO, this is ready for review.

@gabesullice gabesullice changed the title [WIP] Resource Versioning Profile Resource Versioning Profile Dec 20, 2018

- `subsequent-working-copy`: links to the working copy which immediately
preceded the context resource object.

A server **MAY** provide an array of any of these links to support branching.

This comment has been minimized.

@gabesullice

gabesullice Dec 20, 2018

Contributor

@ethanresnick, this is in conflict with the base spec. Not sure what to do or how to accommodate this in any other way. Perhaps you have some ideas?

This comment has been minimized.

@ethanresnick

ethanresnick Dec 21, 2018

Member

@gabesullice I'd be happy to just define these links in the base spec as a temporary workaround. Note also that the base spec's current approach is that, if an array of links is allowed, an array should be required (like we did for profile) to not further proliferate the number of links cases that clients have to handle.

So, the base spec language would go under the resource links section, and could say something like:

If present, this object MAY also contain an array of links as the value for any of the following keys: latest-version, working-copy, predecessor-version, successor-version, [etc]. Links at any of these keys point to resources that are related to the resource object according to the IANA link relation corresponding to their key name.

This comment has been minimized.

@gabesullice

gabesullice Dec 21, 2018

Contributor

That'd be great :)

This comment has been minimized.

@gabesullice

gabesullice Jan 2, 2019

Contributor

bump.

This comment has been minimized.

@wimleers

wimleers Jan 11, 2019

Contributor

@ethanresnick Rather than calling out those 4 specific link relations, perhaps it'd be better to generalize that to something like:

Links with a key that matches a registered link relation type which allows multiple values are allowed to have a value that is not a link, but an array of links.

This comment has been minimized.

@gabesullice

gabesullice Jan 11, 2019

Contributor

While I agree that calling out specific link relations "smells" a bit, I would not like to see the concept of links object keys as link relations furthered in the spec. Links can have more than one link relation and this pattern makes that more challenging to fix in later iterations of the spec.

@ethanresnick

This comment has been minimized.

Copy link
Member

ethanresnick commented Dec 21, 2018

@gabesullice Thanks for this!

  1. This is an awesome profile for a much-needed feature.

  2. It's a great test of the profile review criteria and timeline. (It already reminded me about another criterion we needed.)

My inline comments thus far have been a bit scattered/design focused, but I'll go through this soon with an attempt to apply the formal review criteria.

@ethanresnick
Copy link
Member

ethanresnick left a comment

Current review, by the formal criteria:

  • follows the profile template;
  • is specified precisely enough to enable interoperable implementations [not quite, see comments];
  • complies with the JSON:API spec and its requirements for profiles [not quite, see comments];
  • wouldn鈥檛 cause any problems were it to become widely adopted.
Show resolved Hide resolved _profiles/drupal/resource-versioning/index.md
Show resolved Hide resolved _profiles/drupal/resource-versioning/index.md
**MUST** include the `resourceVersion` query parameter with the same version
identifier that was requested.

When a server processes a request with a `resourceVersion` query parameter

This comment has been minimized.

@ethanresnick

ethanresnick Dec 21, 2018

Member

[Design] My biggest piece of feedback about this profile is that it should probably also consider how to update resources (e.g., how to create a new working copy based of an existing one, or change the default version, or reject a PATCH if the resource's working copy has changed from the working copy that the client is trying to apply its PATCH to, etc). When resource versions have come up in the past, these updating use cases were actually the primary motivation, so not addressing them here would seem like a major missed opportunity that might be hard to remedy once this is published.

I've put this comment on this paragraph, though, because I think the considerations about updating bear directly on what the self link should be. If the self link is URL of a specific revision (as it seems to be here), then a PATCH to that link would be interpreted as updating the revision -- which should probably usually be immutable -- and creating a new revision would become a POST (or a patch to the default version's URL). So that's something to think about.

Also, if each revision is it's own HTTP resource (i.e., has its own URL), then the JSON:API id would usually be different for each revision too (e.g., so a PATCH actually could target the revision), though I suppose it wouldn't have to be. Distinct ids could cause lots of problems, though, because presumably only the default version's type/id would want to be used in relationship objects.

So this is all stuff to think about... I don't have any conclusions at the moment, but I'll mull it over, and hopefully you guys can too. I think, as usual, it goes back to the weird relationship that JSON:API resource objects have with HTTP's concepts of resources + entities.

This comment has been minimized.

@e0ipso

e0ipso Dec 21, 2018

which should probably usually be immutable

I think it makes sense to treat revisions as immutable.


I agree with all that was said in the comment above, but I want to highlight another implicit way of creating revisions that we may need to be explicit about.

Many frameworks handle revision creation as part of the store update. In Drupal when you save an entity that supports revisioning by PATCHing it (no resourceVersion used here), then a new revision will be created for you. You cannot do much about it in most of the cases. This new revision will be a working-copy or a version depending on some custom business logic (usually a published: true / false flag).

I guess that what I'm saying is that strict regulation of how revisions are created may become hard. However, we can put language on how to create revisions through JSON:API.

Where I'm going is, do we want this profile to include revision creation or it can be a separate profile?

This comment has been minimized.

@gabesullice

gabesullice Dec 21, 2018

Contributor

[Not] addressing [PATCH/DELETE] here would seem like a major missed opportunity that might be hard to remedy once this is published.

I think I agree.

Where I'm going is, do we want this profile to include revision creation or it can be a separate profile?

As above, I think I would like this profile to define how update/delete should be handled.


I think the considerations about updating bear directly on what the self link should be. If the self link is URL of a specific revision (as it seems to be here), then a PATCH to that link would be interpreted as updating the revision -- which should probably usually be immutable -- and creating a new revision would become a POST (or a patch to the default version's URL).

I think I may be in a minority about this, but I try to deeply appreciate the difference between a top-level self link and a resource object self link. I feel that once one groks what it means when they're different, the way JSON:API reconciles resource objects with HTTP resources + entities becomes much easier to think about.

Where I'm going with that is pretty simple: the top-level self link should be the URL used to mutate a resource object, not the resource object self link. Once you establish that, things become pretty simple.

GET /article/1?resourceVersion=rel:working-copy

{
  "data": {
    "type": "article",
    "id": 1,
    "links": {
      "self": "/article/1?resourceVersion=id:42"
    }
  },
  "links": {
    "self": "/article/1?resourceVersion=rel:working-copy"
  }
}

That means that a PATCH would update the working copy via the top-level self link, whatever it may be, like so:

PATCH /articles/1?resourceVersion=rel:working-copy

{
  "type": "article",
  "id": 1,
  "attributes": { // some changes }
}

The server response would then be:

200 OK

{
  "data": {
    "type": "article",
    "id": 1,
    "links": {
      "self": "/article/1?resourceVersion=id:43" // <- incremented
    }
  },
  "links": {
    "self": "/article/1?resourceVersion=rel:working-copy" // <- unchanged
  }
}

The working-copy link could be used instead of the top-level self link if the GET request was made directly to a resource object via an over-specific version negotiator (this would be the case if one visited /articles/1?resourceVersion=id:42 directly for some reason).

which should probably usually be immutable

I think it makes sense to treat revisions as immutable.

I think it makes sense too, but I don't think this profile needs to define that. I think it can work either way.

Drupal actually does permit updating a revision in-place (whether that's a good idea or not is beside the point 馃槢 ). With the scheme above, it would be possible for a server to support both in-place editing of a revision or new revision creation via PATCH depending on the self link used (top-level vs. resource object).

If a server cannot automatically create new revisions via PATCH for some reason (maybe it can't/hasn't implemented the rel negotiator), then I think the appropriate URL for creating new revisions would be at the version-history URL via POST. (this is why it makes sense for it to be a collection resource).

This comment has been minimized.

@gabesullice

gabesullice Dec 21, 2018

Contributor

The language about updating resources in the spec seems to jive with what I suggested:

The URL for a resource can be obtained in the self link of the resource object. Alternatively, when a GET request returns a single resource object as primary data, the same request URL can be used for updates.

We could go a step further in that language by adding:

the same request URL or the top-level self link can be used for updates.

This comment has been minimized.

@wimleers

wimleers Jan 11, 2019

Contributor

I agree with @ethanresnick that the fact that this profile's narrowness (only defining "read" behavior) is its biggest weakness.

(Similarly, I think #824's biggest weakness also was its narrowness, it just was different: it focused on optimistic concurrency control.)

On the other hand, I think solid read-only support and leaving modifications to a separate profile or a future iteration is preferable over having a spec that supports fewer use cases (which I think was true of #824it did not allow a particular revision to be retrieved). Especially if it's based on another established standard (RFC5829) and therefore likely to be more implementation-agnostic.

This comment has been minimized.

@wimleers

wimleers Jan 11, 2019

Contributor

I'm fascinated by @gabesullice's proposal (and the sample response bodies really help, thanks! 馃檹). I'm very curious to find out what @ethanresnick thinks about that :)

This comment has been minimized.

@gabesullice

gabesullice Jan 11, 2019

Contributor

or reject a PATCH if the resource's working copy has changed from the working copy that the client is trying to apply its PATCH to

I neglected to give a solid answer for this. As I said above, I think the PATCH should be sent to the top-level self link: /article/1?resourceVersion=rel:working-copy.

However, this spec could add something like an appliesToRevisionId: id:42 under the meta key of the request document.

of a response document from a version history endpoint must be a collection of
resource objects.

Unless an `id` contains version information, the `type` and `id` members of each

This comment has been minimized.

@ethanresnick

ethanresnick Dec 21, 2018

Member

[Spec compliance] Having multiple resource objects in data with the same type/id is sort of a spec violation. I say "sort of" because the spec technically says only that:

A compound document MUST NOT include more than one resource object for each type and id pair.

So, if this version history response isn't a compound document, you're technically in the clear. But the intent has always been that this restriction should apply to all responses. (I can't find the link for that now, but it's come up in conversations with @dgeb over the years.)

Note: I've also proposed removing the restriction altogether.

This comment has been minimized.

@gabesullice

gabesullice Dec 21, 2018

Contributor

It is not intended to be a compound document.

I don't think this profile can impose a requirement that ids contain version information. I think most systems store revision IDs separately from the entity ID. In our case, we use UUIDs for id and revision IDs are auto-incrementing integers. We'd have to make some wonky concatenation of these to put revision IDs into our resource object IDs and I don't think this profile should define how to do that.

I've been considering adding a recommendation that implementations add information to resource object's meta object that matches their most-specific version negotiator. For example, in our case (since we implement the id negotiator) we'd have:

{
  "type": "article",
  "id": "some-long-uuid",
  "meta": {
    "resourceVersionId": 42
  },
  "links": {
    "self": "/articles/some-long-uuid?resourceVersion=id:42"
  }
}

The rule would be something like:

It is RECOMMENDED that resource object's meta object contains a member whose name is resourceVersion appended by the upper-camel-cased name of the version negotiator used in the resource object's self link. The value of this member should be the the version identifier used in the resource object's self link, excluding its first segment, and it MUST NOT begin with a colon.

This comment has been minimized.

@gabesullice

gabesullice Dec 21, 2018

Contributor

I just read the #824 PR. I like the idea of a revision key. This profile could adopt revision as a meta object member and require that it must be equal to the complete version identifier in a resource object's self link (or a valid one if a self link is not provided).

This comment has been minimized.

@gabesullice

gabesullice Jan 2, 2019

Contributor

@e0ipso @wimleers thoughts? ^

This comment has been minimized.

@wimleers

wimleers Jan 11, 2019

Contributor

I also like a revision key (or version). This would also address @ethanresnick's "sort of" spec violation concern: if present, then it would be a higher specificity resource identifier (or perhaps even an intra resource identifier?): just like [type, id] uniquely identify a resource, [type, id, version] uniquely identifies a version of that resource.

Taking this further, there are multiple potential axes along which a variant of a resource can exist: version (or revision) is one, lang (or translation) is another. I think those are the two clearest, strongest use cases. It's possible to think of others though, such as localization.

I'd say versioning and translations are the two most common needs. I think we should keep both in mind if we're going to go this direction.

But for the sake for consistency and optionality (to ensure backwards compatibility & evolvability), I think we should consider not adding a version key, but a variant key. Under variant, this profile could then reserve the version key. A translation profile could reserve the translation key. This also opens the door for multiple versioning profiles if there is the need.

After having written this, I continued reading #824, to make sure I didn't miss anything. I think my variant proposal would actually address the "identity" concerns that @ethanresnick raised in
#824 (comment)? :)

This comment has been minimized.

@gabesullice

gabesullice Jan 12, 2019

Contributor

Crazy thought... maybe we could we extract a variant profile out of this profile? Re-minting resource_version as resource_variant. The "negotiation mechanism" would become the variant negotiation mechanism. Then you could have something like ?resource_variant=lang:en-US or resource_variant=revision:id:42 or ?resource_variant=revision:rel:working-copy.

In fact, if this variation scheme were part of the base spec, then revisions, translations, etc could just enhance that mechanism for its needs.

- `subsequent-working-copy`: links to the working copy which immediately
preceded the context resource object.

A server **MAY** provide an array of any of these links to support branching.

This comment has been minimized.

@ethanresnick

ethanresnick Dec 21, 2018

Member

@gabesullice I'd be happy to just define these links in the base spec as a temporary workaround. Note also that the base spec's current approach is that, if an array of links is allowed, an array should be required (like we did for profile) to not further proliferate the number of links cases that clients have to handle.

So, the base spec language would go under the resource links section, and could say something like:

If present, this object MAY also contain an array of links as the value for any of the following keys: latest-version, working-copy, predecessor-version, successor-version, [etc]. Links at any of these keys point to resources that are related to the resource object according to the IANA link relation corresponding to their key name.


If provided, resource objects' `self` links **MUST NOT** be the same.

# Version Negotiators

This comment has been minimized.

@ethanresnick

ethanresnick Dec 21, 2018

Member

[Design feedback/interoperable implementations review requirement] If I'm understanding correctly, each server implementation can define its own version negotiators, with multiple servers using the same negotiator name in different ways (and no central registry). That seems not great for interoperability. It would also seem to mean that no other version negotiators can ever be standardized as part of this profile, because they could conflict with existing implementations. Is that really what you want?

My gut instinct would be to make this profile's spec the canonical list of all legal version negotiators. Then, you can add more over time to this spec as they're requested. To support people who really need to use a version negotiator that isn't part of your profile, you could have some set of negotiator names/schemes/namespaces that are allowed to have implementation-specific meanings.

This comment has been minimized.

@gabesullice

gabesullice Dec 21, 2018

Contributor

What I've done is applied the implementation-specific query parameter name constraints as a version negotiator constraint and added a mechanism for adding new negotiators to the profile.

Changes here: f0d73db

Elsewhere, @e0ipso suggest that we use URIs but I think this is sufficient. @e0ipso, WDYT?

This comment has been minimized.

@gabesullice

gabesullice Dec 21, 2018

Contributor

cc: @wimleers cause it's his inbox that I suggested too 馃槢

This comment has been minimized.

@e0ipso

e0ipso Dec 21, 2018

I'm OK with that. However I still think that doc curies offer a lower barrier of entry. One could document the new crazy negotiator in their blog and have the server implementation point to that, instead of adding it here.

Many will just not go through the trouble to send their idea to some email addresses and be potentially blocked by them.

This comment has been minimized.

@gabesullice

gabesullice Dec 21, 2018

Contributor

I guess I wanted to discourage non-standard negotiators being used as a standard and keep them isolated. My feeling was that sending an email saying "hey, I'd like to add a negotiator like this..." doesn't seem like a super high barrier. In some ways, it's even less difficult than opening a PR.

What if we add this:

Note: Don't be shy! New version negotiators are more than welcome, the editors want to see this profile proliferate and that means accepting versioning strategies like yours!

This comment has been minimized.

@gabesullice

gabesullice Jan 2, 2019

Contributor

@e0ipso, would that work?

Show resolved Hide resolved _profiles/drupal/resource-versioning/index.md

@gabesullice gabesullice force-pushed the gabesullice:resource-versioning branch from c6c9d21 to ba9bf0b Dec 21, 2018

@wimleers
Copy link
Contributor

wimleers left a comment

I did an initial complete pass. I think it's fascinating how #824 was too narrow in one way (focusing on optimistic concurrency control and hence not even providing the ability to retrieve a particular revision/version) and this is too narrow in another way (not talking about POST/PATCH/DELETE at all).

I do see that @ethanresnick's insightful (and constructively critical! 馃憦 ) comments are resulting in solid improvements. I like where this is going :)

defined as _resource versioning_.
This profile establishes a protocol for resource versioning by defining a
query parameter and its semantics in order to identify arbitrary revisions

This comment has been minimized.

@wimleers

wimleers Jan 11, 2019

Contributor

revisions 鈫 versions :)

This comment has been minimized.

@gabesullice

gabesullice Jan 12, 2019

Contributor

Not in this instance, remember: "not all revisions are versions, but all versions are revisions"

---

# Concepts
Resources on a server may undergo changes and the state of a resource with an

This comment has been minimized.

@wimleers

wimleers Jan 11, 2019

Contributor

I think that by "the state [鈥 may be accessible", you mean that each state may be individually accessible?

arbitrary number of changes may be accessible. It is often useful to retrieve
resources as they existed at the time of their creation or in various states of
change for editorial or archival purposes. This profile establishes a protocol
for accessing resources in those various states.

This comment has been minimized.

@wimleers

wimleers Jan 11, 2019

Contributor

This is talking about previous states, for archival/history purposes. I think this should also explicitly state next states, i.e. drafts that are actively being worked on, or perhaps need to go through legal review, or are ready but simply aren't yet live because they're scheduled to be published in the future.

This comment has been minimized.

@gabesullice

gabesullice Jan 12, 2019

Contributor

This is talking about previous states, for archival/history purposes.

I agree that this could be clarified, but I did not mean previous as you are interpreting it.

Even future revisions were created previously to now. They may not be the default version yet, but that does not mean that the revision itself was created in the future.


A _version_ is to be understood as a revision that is or was the default
revision of a resource. In other words, as a revision of a resource that is
available, or was previously available, without any version negotiation.

This comment has been minimized.

@wimleers

wimleers Jan 11, 2019

Contributor

I think something like "that used to be the canonical representation, i.e. available without any version negotiation" would make this even more clear?

This comment has been minimized.

@gabesullice

gabesullice Jan 12, 2019

Contributor

I very intentionally avoided the word "canonical" here because the canonical link relation is not intended to disambiguate revisions of a resource. It disambiguates duplicates of a resource when they have different URIs.

revision of a resource. In other words, as a revision of a resource that is
available, or was previously available, without any version negotiation.

A _working copy_ is the revision to which new changes can be made or to which

This comment has been minimized.

@wimleers

wimleers Jan 11, 2019

Contributor

This term is specifically borrowed from
https://tools.ietf.org/html/rfc5829, because this entire profile was modeled after RFC5829. Linking to that at the top of the "Concepts" section would be helpful.

This comment has been minimized.

@gabesullice

gabesullice Jan 12, 2019

Contributor

Wow, I can't believe that I didn't already do that.

**MUST** include the `resourceVersion` query parameter with the same version
identifier that was requested.

When a server processes a request with a `resourceVersion` query parameter

This comment has been minimized.

@wimleers

wimleers Jan 11, 2019

Contributor

I agree with @ethanresnick that the fact that this profile's narrowness (only defining "read" behavior) is its biggest weakness.

(Similarly, I think #824's biggest weakness also was its narrowness, it just was different: it focused on optimistic concurrency control.)

On the other hand, I think solid read-only support and leaving modifications to a separate profile or a future iteration is preferable over having a spec that supports fewer use cases (which I think was true of #824it did not allow a particular revision to be retrieved). Especially if it's based on another established standard (RFC5829) and therefore likely to be more implementation-agnostic.

**MUST** include the `resourceVersion` query parameter with the same version
identifier that was requested.

When a server processes a request with a `resourceVersion` query parameter

This comment has been minimized.

@wimleers

wimleers Jan 11, 2019

Contributor

I'm fascinated by @gabesullice's proposal (and the sample response bodies really help, thanks! 馃檹). I'm very curious to find out what @ethanresnick thinks about that :)

- `subsequent-working-copy`: links to the working copy which immediately
preceded the context resource object.

A server **MAY** provide an array of any of these links to support branching.

This comment has been minimized.

@wimleers

wimleers Jan 11, 2019

Contributor

@ethanresnick Rather than calling out those 4 specific link relations, perhaps it'd be better to generalize that to something like:

Links with a key that matches a registered link relation type which allows multiple values are allowed to have a value that is not a link, but an array of links.


This profile establishes the `id` version negotiator. An `id`-based version
identifier is composed of two segments鈥攖he `id` version negotiator and a single
version argument. Any colons (U+003A COLON, 鈥:鈥) present in the version

This comment has been minimized.

@wimleers

wimleers Jan 11, 2019

Contributor

This should say that the version argument is an opaque string.


> Note: This profile is agnostic about the format of the version argument in
> `id`-based version identifiers. For example, one server may use integers as
> revision IDs, another may use UUIDs and yet another may use content-based

This comment has been minimized.

@wimleers

wimleers Jan 11, 2019

Contributor

This note could go away by defining it as an opaque string, like I suggested above.

@gabesullice

This comment has been minimized.

Copy link
Contributor

gabesullice commented Jan 11, 2019

General comment: I think this profile need to be reviewed to ensure that the revision/version distinction is well maintained in naming. I'm thinking of "version identifier", which I think should become "revision identifier".

@wimleers

This comment has been minimized.

Copy link
Contributor

wimleers commented Jan 16, 2019

I agree that that aspect is most confusing.

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