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
Fix for FCREPO-1640 #844
Fix for FCREPO-1640 #844
Conversation
@@ -58,71 +61,36 @@ public TypeRdfContext(final FedoraResource resource, | |||
|
|||
//include rdf:type for primaryType, mixins, and their supertypes | |||
concatRdfTypes(); | |||
if (resource() instanceof NonRdfSource) { | |||
// gather versionability info from the parent | |||
if (resource().getNode().getParent().isNodeType("mix:versionable")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of relying on resource().getNode().getParent()
, it may be more maintainable if you use:
((NonRdfSource) resource()).getDescription().getNode().isNodeType(...)
...which raises the question of versionability of NonRdfSource properties. Do we care if the description is versionable if in fact the properties (stored on the NonRdfSource) are not themselves versionable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the description be versionable if the described isn't? Do we offer that? I don't think we do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure whether your suggestion is more maintainable or not, because we don't want to conflate description and versioning lifecycle. IOW, I'm looking at the parent not because it is the description, but specifically because it is the parent. If it wasn't the description and the description lived elsewhere (which we can easily imagine supporting) I would still be looking at the parent. I wouldn't go and trace through to the description. That's because it is the parent that controls the versionability, not the description. It just happens that for the moment, in this case, they are the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough.
My question becomes, are we loosing versionability of NonRdfSource properties by storing them on the NonRdfSource? In other words, why is the check for "mix:versionable" not being performed on the NonRdfSource?: resource().getNode().isNodeType(...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not being checked there because it's not being stored there. As to why, I do not know. You would have to go investigate that code. I assumed it was for a reasonable reason, but after all, this is Fedora.
I don't think we're losing anything. I don't think we currently can version NonRdfSource
properties independently of the bitstream. Can you demonstrate that we can? I'm also not sure why anyone would want to do that. Fixity, after all, is one of those properties.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification about both the description and the NonRdfSource being versioned. Your code makes it look like the triple stating that the NonRdfSource is versioned is derived from whether the NonRdfSource's parent is versioned. Am I misreading this:
https://github.com/fcrepo4/fcrepo4/pull/844/files#diff-e7befd57ec900402b6ffad300b697af1R66
As far as any desire to have one or the other of the description or NonRdfSource versioned and not the other, I have none. I am sorry if I gave an indication otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is indeed the parent's status that produces the child's triple. This is because they will always be the same (to my understanding). Do you think an inline comment is appropriate here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are correct, @ajs6f. Versioning appears to work as expected on NonRdfSources and their descriptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An additional inline comment would be bonus.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do (will comment, that is). I no longer know what "work as expected" even means any more, so I feel like we're in good shape here.
Closed in favor of: |
No description provided.