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

Restoring versions #217

Closed
escowles opened this issue Sep 13, 2017 · 30 comments

Comments

Projects
None yet
6 participants
@escowles
Copy link
Contributor

commented Sep 13, 2017

The Modeshape implementation currently restores versions using a PATCH request on the version resource. But 4.2.6 LDPRm PATCH says "An implementation must not support PATCH for LDPRms."

Is there an alternate way of restoring versions? Could we change that to "An implementation must not support PATCH for modifying LDPRms." (inserting "modifying")?

@zimeon

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2017

I don't think the modifying approach would make sense without extra wording/specificity around the concept of restoring.

I'm not familiar with the current Modeshape implementation: does restore via patch rely on some secretly saved copy? Or is it a way for a client to (re)write history?

@escowles

This comment has been minimized.

Copy link
Contributor Author

commented Sep 14, 2017

Maybe doing a PUT on the LDPRv with a Content-Location header pointing at the version would be a better way, and avoid the issue of PATCH not being allowed on the LDPRm?

@bbpennel

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2017

I created a pull request to add language related to restoring a version. If you have a moment please take a look and let me know if it sounds reasonable.

@zimeon

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2017

Although I had previously 👍'd @escowles' #217 (comment), I now think that usingContent-Location in the request to indicate restore-by-reference would go against the HTTP definition of Content-Location. I think we need some other by-reference mechanism for both this and the the upload-by-reference use case (mentioned in #210)

@zimeon

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2017

I see the the PR #226 uses Accept-Datetime as the header to indicate PUT-to-restore. As noted in #215 (comment) , I lean toward repurposing Memento-Datetime in requests for this, rather than reinterpreting Accept-Datetime for this non-conneg use case.

@escowles

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2017

Yes, RFC 2616 conveniently said "The meaning of the Content-Location header in PUT or POST requests is undefined; servers are free to ignore it in those cases."

But RFC 7231 says:

An origin server that receives a Content-Location field in a request
message MUST treat the information as transitory request context
rather than as metadata to be saved verbatim as part of the
representation. An origin server MAY use that context to guide in
processing the request or to save it for other uses, such as within
source links or versioning metadata. However, an origin server MUST
NOT use such context information to alter the request semantics.

So we shouldn't be using Content-Location either for redirect or upload-by-reference.

@bbpennel

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2017

@zimeon Thanks for looking at the PR. I am fine with switching the header. I had selected Accept-Datetime since I could see the header being used for negotiation purposes in the same way it is for get requests. For example, the client wanted to restore to a specific point in time that didn't match an exact memento timestamp. In that case, the overlap between the GET and the restore header made sense to me.

I do agree that Memento-Datetime probably makes more sense in the case of PUT to an LDPCv.

@barmintor

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2017

just to clarify, I don't think there's a use case for PUT to LDPCv... the mutable add-able things are the contained resources, right?

@barmintor

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2017

Maybe I'm misunderstanding the abbreviations:

LDPRv: a versioned resource, also a TimeGate

LDPCv: this is a timemap of versions

LDPRm: these are separately identified versions

So mutating methods should only go to the LDPRv (to change, or in some fashion restore a version) or to the LDPCv if it supports client-initiated memento minting.

PATCH must not be allowed on the LDPRm - they're snapshots in time - unless an implementation does not separably identify LDPRm (ie, it is using a single URI for the resource and the mementos). Maybe that unless needs to be clarified in the spec.

I also did not realize the mode impl's pattern there was... no-body PATCH to a version? I find those semantics extremely confusing.

Finally - in the case of non-binary content, the idea of "restoring a version" is kind of fraught. Some of the triples might be synthetic, and "restoring the version" would require modifying 1 or more other resources. In the simpler case of binary content, I'd reconsider the Content-Location strategy with PUT. I don't think that's altering the semantics - a PUT is still a replacement, right?

@escowles

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2017

I think what would be really helpful here is a concise outline of operations that a client-managed versioning Fedora should support and what operation on what resource would accomplish them. I'm going to make a rough stab at that here:

  1. Check if a resource is versionable and discover the TimeMap/LDPCv
    1. HEAD on the LDPR: Link rel="type" VersionableResource indicates versioning support, Link rel="timemap" points to LDPCv/TimeMap
  2. Check if we can create versions
    1. OPTIONS on LDPCv/TimeMap: Allow: POST indicates that versions can be created
  3. Create a new version
    1. POST to LDPCv/TimeMap with Memento-Datetime header and body to create a historic version with the specified body
    2. POST to LDPCv/TimeMap with Memento-Datetime header and no body to create a historic version with the current state of the LDPRv
    3. POST to the LDPCv/TimeMap with no Memento-Datetime header or body to create version with current time and state of the resource
  4. List existing versions:
    1. GET to LDPCv/TimeMap with Accept: application/link-format for link-value serialization
    2. GET to LDPCv/TimeMap without Accept header (or with an RDF type) for an RDF serialization
  5. Retrieve an existing version:
    1. GET to LDPRv/TimeGate with Accept-Datetime header
    2. Or: GET to LDPRm/Memento
  6. Delete an existing version:
    1. DELETE to LDPRm/Memento
  7. Restore an existing version:
    1. PUT to LDPRv/TimeGate with Memento-Datetime header indicating the version to restore
@bbpennel

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2017

@escowles would 7. Restore an existing version require the client to PUT a body containing the properties of the LDPRm being restored (or possibly content-location header)? LDP appears to require that you replace the body of the target LDPR with body of the PUT. So if you had to first retrieve the LDPRm and then PUT it back to the LDPRv, it seems like that would defeat the point of having a restore end point using datetime negotiation.

@zimeon

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2017

Excellent summary @escowles !

I'm not quite sure about 3.ii. and 3.iii -- it seems odd in 3.ii. to give an (arbitrary) new time and in 3.iii to use the current time. Wouldn't one want to use the stored timestamp of the current LDPRv as the Memento-Datetime of the new memento? (I realize that can be done by HEAD then POST in the scheme proposed, just not sure about utility of other options)

@birkland

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2017

also, I think type VersionalbleResource (in 1.i) is a modshape-impl-ism, unless it's intended to be defined in the spec

@escowles

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2017

@zimeon I think I was getting confused about the different options based on whether there is a body in the POST or not. So I think 3 should be:

  1. Create a new version
    1. POST to LDPCv/TimeMap with Memento-Datetime header and body to create a historic version with the specified body and datetime
    2. POST to LDPCv/TimeMap with a body and no Memento-Datetime header to create a version with the specified body and the current datetime
    3. POST to LDPCv/TimeMap with Memento-Datetime header and no body to create a historic version with the current state of the LDPRv and the specified datetime
    4. POST to the LDPCv/TimeMap with no Memento-Datetime header or body to create version with current state of the resource and the resource's last-modified date as the datetime
@escowles

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2017

@birkland I mean http://fedora.info/definitions/fcrepo#VersionedResource — which is in the spec now, but not published in the ontology yet.

@birkland

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2017

@escowles Ah, I see it now, thanks

@zimeon

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2017

@escowles re. #217 (comment) I think that makes sense now. I think i) and iv) are the key cases

@escowles

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2017

@bbpennel I agree that doing a PUT to the LDPRv to restore a version seems to conflict with the LDP requirements. So we either need to figure out a different operation for that, or some header to indicate that the content comes from the version instead of the request body. Unfortunately, Content-Location can't be used because the HTTP spec now says it identifies the body of the POST, not replaces it.

@escowles

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2017

I've updated my list and put it in a gist to make it easier to edit it: https://gist.github.com/escowles/69b8aeac25e4a3839df0710e03378c6a

I think the big remaining piece is exactly how to restore versions. 7.iii seems like the best option here, if we can agree on a header for "use content from this URI" (which we also need for the ingest-by-reference use case).

@bseeger

This comment has been minimized.

Copy link

commented Sep 21, 2017

@escowles - what use case does this cover? POST to LDPCv/TimeMap with a body and no Memento-Datetime header to create a version with the specified body and the current datetime
Really, I just don't get when one would use this, but I don't know what all people are doing with versioning. It just seems odd to use potentially different content to make a current memento of something that may not match the memento at the current point in time.

@escowles

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2017

@bseeger The spec says (2nd paragraph):

If an LDPCv does accept POST with a request body, it should respect a Memento-Datetime request header for the created LDPRm. Absent this header, it must use the current time.

So I'm not sure exactly what the semantics of that are, but I think it's required. Maybe it is effectively the same as doing a PUT to the LDPRv?

@bseeger

This comment has been minimized.

Copy link

commented Sep 21, 2017

Also GET to LDPCv/TimeMap without Accept header (or with an RDF type) for an RDF serialization - would you expect this to be RDF serialization of the TimeMap or the contents of the LDPCv itself? The LDPCv contents could contain more then what the TimeMap requires (since LDPCv allows for PATCH on it so longs as it doesn't change the containment triples).

@bseeger

This comment has been minimized.

Copy link

commented Sep 21, 2017

@escowles - interesting - I see what you mean.

@bseeger

This comment has been minimized.

Copy link

commented Sep 21, 2017

@escowles - I'm slowing bringing your gist file into the design document that @bbpennel and I have ben working on. Do you mind if we transition to that for any future changes/updates?.

@escowles

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2017

@bseeger Yes, I think the wiki is probably a better place for this than my gist.

And I don't think the spec has any position on what other representations of the TimeMap contain — but for Modeshape, I would expect it to be a list of the versions, plus any other properties that are attached to the LDPCv (from PATCH updates or wherever).

@bseeger

This comment has been minimized.

Copy link

commented Sep 21, 2017

@escowles: in regards to HEAD on the LDPR: Link rel="type" http://fedora.info/definitions/fcrepo#VersionedResource indicates versioning support, Link rel="timemap" points to LDPCv/TimeMap <-- I don't see where in the spec it specifies that the Link rel="type" http://fedora.info/definitions/fcrepo#VersionedResource needs to be included on a GET on a LDPRv. Is that part of the CRUD changes - to include "type" links for resource type? I guess I just want to understand what this means more. Kind of an unclear question and we can certainly do that even if it's not in the spec. I thought that having a timemap / gateway link would be enough to indicate that it's versioned.

Also, @bbpennel and I moved the content of the gist into here: https://wiki.duraspace.org/pages/viewpage.action?pageId=90964507

@escowles

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2017

@bseeger You're right that the spec doesn't say that you have to include the VersionedResource in the GET responses — it just says that if you include it when you create a LDPR, then that LDPR should have an LDPCv, etc. So I'm mostly just assuming that if you create a resource with the VersionedResource type, that GET/HEAD responses for that resource would include that and other types (like the LDP interaction model). But maybe that's not a good assumption (or should be explicitly required if we think that is a good thing to do).

@bseeger

This comment has been minimized.

Copy link

commented Sep 21, 2017

@escowles - I'm fine w/ doing it but not having it required by the spec. I think it's probably handy to have in the headers, but not required since there are other indicator links there too.

@escowles

This comment has been minimized.

Copy link
Contributor Author

commented Nov 1, 2017

Closing in favor of discussion in #238

@escowles escowles closed this Nov 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.