Disallow tombstone purge in AG and allow PUT overtop of tombstones

[mikejritter]

JIRA Ticket:

• https://fedora-repository.atlassian.net/browse/FCREPO-3881

• https://fedora-repository.atlassian.net/browse/FCREPO-3883

• Other Relevant Links (Mailing list discussion, related pull requests, etc.)

• https://fedora-repository.atlassian.net/browse/FCREPO-3866

What does this Pull Request do?

This PR covers two tickets for updating the behavior of tombstones, which came from discussions of how Archive Groups behave and how to update for certain scenarios (e.g. PUT-DEL-PUT). The main behavior changes are disallowing DELETEs of tombstones in Archival Groups and allowing PUT overtop of tombstones for both AGs and Atomic resources.

• Removes ability to purge tombstones in Archive Groups
• Allow retrieval of tombstones when searching for resources
• Add Override-Tombstone header
• Update precondition check to use deleted resource if a tombstone exists
• Update PUT behavior to allow tombstones to be overwritten
• Update test for purging tombstones to expect a 405
• Update create/del/purge test for new responses and add Overwrite-Tombstone header
• Add test for changing the interaction model of a resource when putting over a tombstone

How should this be tested?

I had a couple of test cases for validating behavior:

3881 - DELETE of Tombstone in an AG

• Purge in Archive Group - Response is a 405

3883 - PUT overtop of Tombstone

• Test in an AG with Overwrite-Tombstone: true header set - should return 201
  • created_at and last_modified_at should be validated on the new resource
• Test in an AG with Overwrite-Tombstone: false header set - should return 410 (this is the response we currently give)
  • Looking at the diff I missed adding an IT for this case, though I had it in my scripts. Something I'll need to do before merging.
• Test in an AG and change the interaction model, e.g.
  • Initial put could be a binary
  • PUT overtop could change to a container: Link: <http://www.w3.org/ns/ldp#DirectContainer>;rel=\"type\"
  • Response should be a 409
• The same tests can also be run against an atomic resource instead of an archive group, though each go through the same logic

Interested parties

Tag (@ mention) interested parties or, if unsure, @fcrepo/committers

[whikloj]

This works as described. I did note that the OPTIONS for tombstones of AG children still only show DELETE.

> curl -u fedoraAdmin:fedoraAdmin -XOPTIONS -i http://localhost:8080/rest/new_ag2/ag_child_delete/fcr:tombstone
HTTP/1.1 200 OK
Date: Fri, 15 Sep 2023 20:44:33 GMT
Set-Cookie: JSESSIONID=node0vx6o8r0p1s67b4937oddb5km22.node0; Path=/
Expires: Thu, 01 Jan 1970 00:00:00 GMT
Set-Cookie: rememberMe=deleteMe; Path=/; Max-Age=0; Expires=Thu, 14-Sep-2023 20:44:33 GMT; SameSite=lax
Allow: DELETE
Content-Length: 0
Server: Jetty(9.4.44.v20210927)

I'm not sure but it should probably be PUT, except it only works with the required header.

[mikejritter]

@whikloj the PUT is going on the resource so do we need to update the allow header for the tombstone? I guess it's a little odd because you get the tombstone back from querying the resource, and that's also where I put the logic doing a PUT over top of the tombstone. I think it's ok to omit PUT as it isn't happening on the tombstone itself.

Actually another odd thing with the allow headers, for archive groups DELETE won't be allowed so I think we would want to remove it for that case. Otherwise I suppose we could change the response code, even though 405 does seem to make the most sense.

[mikejritter]

@whikloj I just pushed a commit to update the OPTIONS when querying on /fcr:tombstone so that it will have an empty Allow header for archive group members. A few things to note that changed:

• Querying a path which doesn't have an ocfl resource will return a 404
• Querying a path which has not been deleted will return a 404

For both of these I'm not sure if we want to update to maybe return a 204 (or 200) + empty Allow header, or leave as is.

I also checked for deleting the tombstone of an archive group and saw it is tested for in FedoraTombstonesIT.

[whikloj]

Functionally this works, just some questions.

[whikloj]

isBinaryTombstone() replicates the actions above (line 480-483) and then compares resource instanceof Binary.
Here we are testing resource() and resource could we just do

if ((resourceExists && resource instanceof Binary) ||

[mikejritter]

I think the use of resource() everywhere made me want to stick with that, but overall I see what you're saying. I'll see about refactoring a bit so that we can use resource so that we don't need to re-check for conditions we've already met.

[whikloj]

This works for OPTIONS requests, though for Archival Group members you get a blank header

Allow:

But we still return

Allow: DELETE

for all the other methods that return a 406 Method Not Allowed. This is built in the methodNotAllowed() on lines 147-149.

[mikejritter]

@whikloj I just finished making changes based on your feedback, everything should be resolved now. I double checked the headers when returning the 405 on fcr:tombstone and things look good (DELETE exists for non-ag; omitted for ag).

[whikloj]

Sorry @mikejritter this one has many layers.

Everything works... except PUT over the tombstone of an archivalgroup.

That results in a 500 Server Error with a stacktrace of

WARN 08:23:35.896 [qtp1756600221-526] (WildcardExceptionMapper) Unmapped exception
org.fcrepo.storage.ocfl.exception.ValidationException: Resource info:fedora/test_archival_groups/5d35f634-ce53-4706-9578-2965fa21dbba is not a valid Fedora 6 resource. The following problems were identified:
  1. Must define property 'archivalGroupId' as 'null' but was 'info:fedora/test_archival_groups/5d35f634-ce53-4706-9578-2965fa21dbba'
	at org.fcrepo.storage.ocfl.exception.ValidationException.createForResource(ValidationException.java:39)
	at org.fcrepo.storage.ocfl.DefaultOcflObjectSession.validateHeaders(DefaultOcflObjectSession.java:650)
	at org.fcrepo.storage.ocfl.DefaultOcflObjectSession.stageHeaders(DefaultOcflObjectSession.java:631)
	at org.fcrepo.storage.ocfl.DefaultOcflObjectSession.writeResource(DefaultOcflObjectSession.java:205)
	at org.fcrepo.persistence.ocfl.impl.FcrepoOcflObjectSessionWrapper.lambda$writeResource$0(FcrepoOcflObjectSessionWrapper.java:69)
	at org.fcrepo.persistence.ocfl.impl.FcrepoOcflObjectSessionWrapper.exec(FcrepoOcflObjectSessionWrapper.java:195)
	at org.fcrepo.persistence.ocfl.impl.FcrepoOcflObjectSessionWrapper.lambda$writeResource$1(FcrepoOcflObjectSessionWrapper.java:69)

[whikloj]

I realize that the above was me trying to put a normal RDFResource over top the tombstone of an ArchivalGroup, so I also tried PUTting an ArchivalGroup on the tombstone of the ArchivalGroup and that returns 409 Conflict with the stacktrace of

DEBUG 08:27:51.537 [qtp1756600221-542] (PersistentItemConflictExceptionMapper) PersistentItemConflictExceptionMapper intercepted exception:
org.fcrepo.persistence.api.exceptions.PersistentItemConflictException: Nesting an ArchivalGroup within an ArchivalGroup is not permitted
	at org.fcrepo.persistence.ocfl.impl.CreateRdfSourcePersister.persist(CreateRdfSourcePersister.java:55)
	at org.fcrepo.persistence.ocfl.impl.OcflPersistentStorageSession.persist(OcflPersistentStorageSession.java:167)
	at org.fcrepo.persistence.ocfl.impl.OcflPersistentStorageSessionMetrics.lambda$persist$0(OcflPersistentStorageSessionMetrics.java:56)
	at io.micrometer.core.instrument.composite.CompositeTimer.record(CompositeTimer.java:79)
	at org.fcrepo.persistence.ocfl.impl.OcflPersistentStorageSessionMetrics.persist(OcflPersistentStorageSessionMetrics.java:55)
	at org.fcrepo.kernel.impl.services.CreateResourceServiceImpl.perform(CreateResourceServiceImpl.java:194)
	at org.fcrepo.http.api.FedoraLdp.lambda$createOrReplaceObjectRdf$9(FedoraLdp.java:573)

[whikloj]

In case it helps, I have a Python based set of some tests. I used to use it with Fedora 4 and 5. Trying to get it sorted out for F6, but I added an archivalgroup set of tests and this is the one failing.

https://github.com/whikloj/fcrepo4-tests/blob/fcrepo-6/archival_group_tests.py#L149-L190

[mikejritter]

@whikloj I pushed some commits to handle the case of putting over top of an ArchivalGroup's tombstone. I opted to create a new OverwriteRdfTombstonePersister as well as a new resource operation OVERWRITE_TOMBSTONE which both can be thought of as subsets of CREATE. This way the logic in CreateRdfSourcePersister remains the same and doesn't need to worry about the overwrite op.

A quick overview of the changes:

• Add OVERWRITE_TOMBSTONE resource operation
• Add OverwriteRdfTombstonePersister
• Add OverwriteRdfTombstoneOperation as a subclass of CreateRdfSourceOperation
• Add isOverwrite to CreateRdfSourceOperationBuilder to specify a tombstone is being overwritten

[whikloj]

For the future in the interest of speed, would it be faster to set something like

boolean $isTombstone = resource instanceof Tombstone;

up at line 482, then we won't have to reload the object here or below to determine if the original resource was a tombstone?

[whikloj]

Maybe just adding a TODO: to check this out, because it is possible that the caching in the OCFL layer makes this a non-issue.

[mikejritter]

I feel like I was going to use isTombstoneOverwrite here. Not sure why I didn't, we know it should have been checked because both places also rely on resourceExists to be true. IDK I might look into this a bit more and make a new ticket/PR.