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

Fcrepo 2975 - Create timemap when resource created #1504

Merged
merged 2 commits into from
Jan 16, 2019

Conversation

bbpennel
Copy link
Collaborator

@bbpennel bbpennel commented Jan 8, 2019

JIRA Ticket: https://jira.duraspace.org/browse/FCREPO-2975

What does this Pull Request do?

Resolves an NPE when committing transactions containing HEAD and DELETE requests to the same resource by creating timemaps at the same time that the original resource was created.

What's new?

Creating timemap at the same time that a resource is created.

  • Implemented TimeMapService for the creation of timemaps
  • Changed FedoraResourceImpl.getTimeMap() to never create timemaps
  • Allow for responses for versioning disabled resources to not return a timemap uri
  • Moved constant for fedora:timemap node to constants class for consistency and since it did not primarily belong to FedoraResourceImpl anymore.
  • Added and updated tests

How should this be tested?

See example in ticket as well as new integration tests.

Additional Notes:

This changes the approach in fcrepo5 regarding when timemaps are created from lazy-creation to explicit creation at object creation time.

Interested parties

@awoods @dbernstein

* Implemented TimeMapService for the creation of timemaps
* Changed FedoraResourceImpl.getTimeMap() to never create timemaps
* Allow for responses for versioning disabled resources to not return a timemap uri
* Moved constant for fedora:timemap node to constants class
* Added and updated tests
@dbernstein dbernstein self-requested a review January 10, 2019 16:56
Copy link
Contributor

@dbernstein dbernstein left a comment

Choose a reason for hiding this comment

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

@bbpennel : This looks good to me and it works as advertised. Thanks for the quick turn around time.
Just curious: did you see/consider any other ways of preserving the lazy timemap creation? I know we went that root for the sake of performance (although I don't know if we ever quantified the time saved by using lazy time map init.

@bbpennel
Copy link
Collaborator Author

@dbernstein I didn't explore other options too much. The previous implementation was actually creating the time map at the end of the request to create the original resource because of the getTimeMap() call here https://github.com/fcrepo4/fcrepo4/blob/master/fcrepo-http-api/src/main/java/org/fcrepo/http/api/ContentExposingResource.java#L549
But the created timemap was not being saved, so every request to the resource would create the resource again. The timemap only became real when a version of the resource was created because it was committed in that case.

I did consider explicitly committing the session after the lazy creation would have happened here:
https://github.com/fcrepo4/fcrepo4/blob/master/fcrepo-http-api/src/main/java/org/fcrepo/http/api/FedoraLdp.java#L696
But thought that might seem odd and more likely to get undone in the future since retaining the timemap required a second seemingly unrelated step (session.commit())

The other component was that I agreed with @awoods that it seemed undesirable for a HEAD request to make changes to a resource, an unexpected side effect which was partially causing the bug in this ticket. I'm not sure if the head request caused time stamps to change, didn't attempt to verify that.

@dbernstein
Copy link
Contributor

@bbpennel : thanks for the explanation. That all makes a lot of sense.

@dbernstein dbernstein merged commit 0e3092e into fcrepo:master Jan 16, 2019
dbernstein pushed a commit to dbernstein/fcrepo that referenced this pull request Jan 16, 2019
* Add failing test for issue with making a HEAD followed by DELETE request against binary within a transaction

* Creating timemap at the same time that a resource is created.
* Implemented TimeMapService for the creation of timemaps
* Changed FedoraResourceImpl.getTimeMap() to never create timemaps
* Allow for responses for versioning disabled resources to not return a timemap uri
* Moved constant for fedora:timemap node to constants class
* Added and updated tests

Resolves: https://jira.duraspace.org/browse/FCREPO-2975
@bbpennel bbpennel deleted the fcrepo-2975 branch January 16, 2019 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants