Circular collection nesting was not prevented in Hyrax

[laritakr]

Error

Collection was not prevented from having same collection as both a parent and a child in Hyrax (Collections Sprint).

Steps to reproduce in Hyrax (Collections Sprint)

1. Create collection A
2. Create collection B
3. Add collection A as a sub-collection of collection B
4. Add collection B as a sub-collection of collection A.

Rationale

Collection B should not be able to be added as a sub-collection of Collection A, as this is a violation of PCDM.

Expected Behavior

The nested indexer has been updated due to its unexpected behavior in this situation, but PCDM should not be allowing the situation to occur. There should be an error thrown if collections B and A have any sort of ancestral relationship.

Hyrax screen shot

This shows invalid relationships added through UI in collections sprint branch.

[jcoyne]

Collection B should not be able to be added as a sub-collection of Collection A, as this is a violation of PCDM.

@laritakr can you cite where you are seeing that cycles are prohibited in PCDM?

[laritakr]

I was simply told by @jeremyf and @elrayle to add this as a bug here, and that it shouldn't be allowed. I'm not familiar with PCDM to know if it is in fact prohibited.

[jcoyne]

Here's the documentation on PCDM: https://github.com/duraspace/pcdm/wiki I don't see anything about cycles being disallowed.

[laritakr]

The only place I saw that implied the validation was here: https://github.com/samvera/hydra-pcdm/blob/master/lib/hydra/pcdm/models/concerns/collection_behavior.rb#L6

[escowles]

I don't think there's anything in PCDM that says you can't have circularity in membership relationships.

But I know we've prohibited that before (I'm guessing to help avoid infinite recursion).

[jcoyne]

I'm not saying that this isn't a valid issue, but maybe we should strike the part that says: "as this is a violation of PCDM."

[laritakr]

Doesn't this try to validate the ancestry? https://github.com/samvera/hydra-pcdm/blob/master/lib/hydra/pcdm/validators/ancestor_validator.rb

Perhaps the switch in membership direction in Collections Sprint causes the problem?

[elrayle]

When we originally created PCDM, there was a validation that prevented a collection from existing in its own ancestor chain. The tests still check that ancestor rule isn't violated...

https://github.com/samvera/hydra-pcdm/blob/master/spec/hydra/pcdm/models/collection_spec.rb#L145-L184

I did not explore deeper to see if the reason these tests still pass, but @laritakr use fails is due to the switch to reverse the relationship.

[jeremyf]

If this is not something we implement in Hydra::PCDM, I believe the nesting indexer adapter could be extended to include that check. The basic implementation is to leverage the SOLR documents for verification (as opposed to the slow Fedora mechanism which probably should also be implemented).

[jrgriffiniii]

After discussing this with @laritakr, I have added this as an agenda item for the next scheduled Samvera Tech. Call on 08/07/19. While it does not appear to be the case that creating a Hyrax issue to address this within the NestingIndexingAdapter is needed, this still may need to be fixed given what was referenced in #245 (comment)

[tpendragon]

@elrayle We discussed this ticket a bit on the tech call this week, and we're wondering what your opinions would be about closing this ticket given that the PCDM specification doesn't block circular relationships and Hyrax already has the appropriate safeguards?

[elrayle]

The only reason I am hesitant to just close this is it represents a regression from the original functionality. But if others are not concerned about this and it is not causing problems in apps, I will not hold up closing it.