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

Circular collection nesting was not prevented in Hyrax #245

Open
laritakr opened this issue Nov 17, 2017 · 12 comments
Open

Circular collection nesting was not prevented in Hyrax #245

laritakr opened this issue Nov 17, 2017 · 12 comments
Labels

Comments

@laritakr
Copy link

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.
screen shot 2017-11-17 at 10 20 25 am

@jcoyne
Copy link
Member

jcoyne commented Nov 17, 2017

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
Copy link
Author

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
Copy link
Member

jcoyne commented Nov 17, 2017

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

@laritakr
Copy link
Author

laritakr commented Nov 17, 2017

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
Copy link
Contributor

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
Copy link
Member

jcoyne commented Nov 17, 2017

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
Copy link
Author

laritakr commented Nov 17, 2017

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
Copy link
Contributor

elrayle commented Nov 17, 2017

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
Copy link
Contributor

jeremyf commented Nov 17, 2017

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
Copy link
Contributor

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
Copy link
Contributor

@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
Copy link
Contributor

elrayle commented Aug 8, 2019

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.

@jrgriffiniii jrgriffiniii removed the pcdm label May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Backlog
Development

No branches or pull requests

7 participants