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

Fix for FCREPO-1640 #844

Closed
wants to merge 1 commit into from
Closed

Fix for FCREPO-1640 #844

wants to merge 1 commit into from

Conversation

ajs6f
Copy link
Contributor

@ajs6f ajs6f commented Jul 17, 2015

No description provided.

@@ -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")) {
Copy link

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link

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(...)

Copy link
Contributor Author

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.

Copy link

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.

Copy link
Contributor Author

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?

Copy link

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.

Copy link

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.

Copy link
Contributor Author

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.

@ajs6f ajs6f closed this Jul 24, 2015
@ajs6f ajs6f reopened this Jul 24, 2015
@ajs6f
Copy link
Contributor Author

ajs6f commented Jul 24, 2015

Closed in favor of:

#855

@ajs6f ajs6f closed this Jul 24, 2015
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