FCREPO-1627 (https://jira.duraspace.org/browse/FCREPO-1627) #841
Conversation
| @@ -271,6 +271,8 @@ public Response createOrReplaceObjectRdf( | |||
|
|
|||
| if (httpConfiguration.putRequiresIfMatch() && StringUtils.isBlank(ifMatch) && !resource.isNew()) { | |||
| throw new ClientErrorException("An If-Match header is required", 428); | |||
| } else if (resource.hasType(FEDORA_PAIRTREE)) { | |||
| throw new ClientErrorException("Graphs cannot be created for resources under pairtree nodes", FORBIDDEN); | |||
There was a problem hiding this comment.
Do we want something more semantically specific here?
There was a problem hiding this comment.
@ajs6f, are you suggesting a new, more specific exception type?
There was a problem hiding this comment.
Ye, something like CannotModifySystemResourceException except with a snappier title.
There was a problem hiding this comment.
I would please propose the following in response to this:
- ForbiddenResourceModificationException
- UnsupportedResourceModificationException
- IllegalResourceModification
- BadResourceModification
My preference lies primarily with the term "Forbidden" (the mapping to the HTTP 403 response description feels appropriate), but I could certainly defend "Unsupported" as well.
"Illegal" and "Bad" concern me given that they may easily be applicable within situations in which, I would speculate, a 405 response is appropriate. "Cannot" also concerns me in this regard (as well as it being identified as undesirable).
There was a problem hiding this comment.
ForbiddenResourceModificationException is fine with me.
|
This looks good to me. @ajs6f, let me know if your review is done, then I will squash and merge tonight. |
| @@ -271,6 +273,9 @@ public Response createOrReplaceObjectRdf( | |||
|
|
|||
| if (httpConfiguration.putRequiresIfMatch() && StringUtils.isBlank(ifMatch) && !resource.isNew()) { | |||
| throw new ClientErrorException("An If-Match header is required", 428); | |||
| } else if (resource.hasType(FEDORA_PAIRTREE)) { | |||
| throw new ForbiddenResourceModificationException("Graphs cannot be created for resources " + | |||
| "under pairtree nodes"); | |||
There was a problem hiding this comment.
I'm a little confused here, but shouldn't this read "Graphs cannot be created for resources that are pairtree nodes" and wouldn't it be better to have one string on one line?
There was a problem hiding this comment.
Personally, I would prefer wording along the lines of, '"Resources cannot be created under pairtree elements."'
There was a problem hiding this comment.
I'm not worried about the word "graph" and if you like it better without it, fine by me. I'm confused about whether at this moment in the workflow the client is trying to create anything under resource or is trying to create resource or what. I just want to make sure a) that this test is coming at the right time in the workflow and b) that it is throwing out an exception with the right message.
There was a problem hiding this comment.
POST without a slug creates a resource as a child of the POST target container.
e.g. POST /rest/test
results in:
/rest/test/new-resource
PUT creates a resource as the PUT target itself.
e.g. PUT /rest/test
results in:
/rest/test
Note: The wording of the exception here should probably be in line with the one provided when trying to PUT over an existing pairtree element. See: https://jira.duraspace.org/browse/FCREPO-1505
...although I prefer the wording discussed here versus that found in FCREPO-1505.
There was a problem hiding this comment.
Okay, cool then. How about "Resources cannot be manually created under pairtree elements!"
add created = fedora:created :: xsd:dateTime; lastModified = fedora:lastModified :: xsd:dateTime; hasParent = fedora:hasParent :: xsd:string; Resolves: https://jira.duraspace.org/browse/FCREPO-1552
- Response to code review comments
using modeshape's public API
* Add unit tests for FedoraTypesUtils::isExternalNode * Make GetCacheManager class package-protected * Remove GetBinaryStore class, as it can be inlined in GetClusterConfiguration class Related to: https://jira.duraspace.org/browse/FCREPO-1535
… code for attempts to create graphs using PUT requests for resources under pairtree Nodes within FedoraLdp#createOrReplaceObjectRdf; Implementing FedoraLdpIT#testPutOnPairtree for the case in which clients attempt to PUT graphs under pairtree Nodes
- Use ebucore:filename for premis:hasOriginalName - Use ebucore:hasMimeType for jcr/fedora:mimeType Resolves: https://jira.duraspace.org/browse/FCREPO-1532
- Add minor exclusion for jacoco analysis Resolves: https://jira.duraspace.org/browse/FCREPO-1643
- Add IT to cover typical PCDM use case of proxies Resolves: https://jira.duraspace.org/browse/FCREPO-1497
rename org.fcrepo.kernel.impl package to org.fcrepo.kernel.modeshape
…tion message within FedoraLdp#createOrReplaceObjectRdf; Ensuring that attempts to create Objects under pairtree nodes within FedoraLdp#createObject raises an instance of ForbiddenResourceModificationException
…onse.Status.FORBIDDEN in compliance with checkstyle
- Add UniqueValueSupplier Resolves: https://jira.duraspace.org/browse/FCREPO-1642
Move exception handling from FedoraLdp into ContentExposingResource
Correcting problematic hasVersions triple Rebase-age
Related to: https://jira.duraspace.org/browse/FCREPO-1640 response to code review comments
…OnPairtree verifies that transmitting a PUT request for "[pairtree]/test" raises the 403 status code
…tatus.FORBIDDEN import
|
After failed attempts to rebase the fcrepo-1627 branch, this pull request is deprecated by #901. |
Raising a ClientErrorException with a 403 status code for attempts to create graphs using PUT requests for resources under pairtree Nodes within FedoraLdp#createOrReplaceObjectRdf; Implementing FedoraLdpIT#testPutOnPairtree for the case in which clients attempt to PUT graphs under pairtree Nodes