search

samvera / hyrax

Hyrax is a Ruby on Rails Engine built by the Samvera community. Hyrax provides a foundation for creating many different digital repository applications.
http://hyrax.samvera.org/
Apache License 2.0
182 stars 122 forks source link

Change default admin set id to `admin_set_default` to avoid issues with encoded slashes #6825

Closed abelemlih closed 1 week ago

abelemlih commented 1 month ago

Fixes

Fixes Fedora 6.5+ compatibility, since it does not support encoded slashes in IDs.

Summary

We currently cannot use Fedora 6.5+ with Hyrax because it does not support encoded slashes in IDs. This leads to failure every time we perform Hyrax.query_service.find_by(id: DEFAULT_ID). In this PR, I change the DEFAULT_ID to use underscores only, and add backwards compatibility changes to helper methods finding existing admin sets.

Type of change (for release notes)

Detailed Description

After delving into the use of DEFAULT_ID , it’s only ever used as an actual ID in places that have a hardcoded admin set with that ID, which is not the case for Postgres or Fedora (Fedora uses UUID). The reasoning behind my PR is, to support backwards compatibility, when we need to lookup any existing unused admin set with default id, lookup for both admin_set/default and admin_set_default, if none exist then go forward with creating a new admin set, which will use UUID with Valkyrie. This change ensures new apps using underscores if they ever have to create a default admin set with DEFAULT_ID , and supports any existing apps that do not use Postgres/Fedora via Valkyrie and have an admin set with the id explicitly set to admin_set/default.

one edge case to this approach is, if there is a repository that has a default admin set in Fedora that has an id explicitly set to admin_set/default , then it will still break, because Fedora 6.5+ will break every time it looks it up. My understanding is we store the default admin set id in Fedora with UUID, and we store the UUID in the hyrax_default_administrative_set on the database.

Changes proposed in this pull request:

@samvera/hyrax-code-reviewers

github-actions[bot] commented 1 month ago

Test Results

    17 files  ±0      17 suites  ±0   2h 19m 59s :stopwatch: + 1m 47s  6 703 tests  - 1   6 406 :white_check_mark: +4  297 :zzz: ±0  0 :x:  - 5  13 175 runs  ±0  12 780 :white_check_mark: +5  395 :zzz: ±0  0 :x:  - 5 

Results for commit 591dd33e. ± Comparison against base commit d4e646c7.

This pull request removes 267 and adds 266 tests. Note that renamed tests count towards both. ``` spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to create # spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to create # spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to create # spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to create # spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to destroy AdminSet: 90fb19b8-68bd-459b-a4b5-9fab8060ec27 spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to destroy Hyrax::AdministrativeSet: 6273321f-27b1-49f0-b1fc-8785918b7503 spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to edit AdminSet: 031a2f29-6273-42cb-85b3-62ca8ccd9005 spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to edit Hyrax::AdministrativeSet: 08c8b9f1-14db-4162-bb1e-52bdbe1b4ab1 spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to update AdminSet: 5586af41-c78e-4c19-b991-071ab16f3e47 spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to update Hyrax::AdministrativeSet: 9506ac69-d58e-4e78-b739-14dc38101667 … ``` ``` spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to create # spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to create # spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to create # spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to create # spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to destroy AdminSet: a8b495cf-f2ef-49cf-8b95-58e2c886fa1a spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to destroy Hyrax::AdministrativeSet: ed4a9fa4-ebea-4ac3-a138-d28550931f02 spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to edit AdminSet: 505083a7-2eef-4d35-9ce2-edd472e0df69 spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to edit Hyrax::AdministrativeSet: 969f2be8-9df3-47e7-8546-3d87a265f8c9 spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to update AdminSet: 15bfc0ad-507a-470c-a4b0-23fc15364ecc spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to update Hyrax::AdministrativeSet: dc9d8982-4975-409d-8579-9108c618b223 … ```

:recycle: This comment has been updated with latest results.

abelemlih commented 4 weeks ago

@randalldfloyd thanks for looking into this, I really appreciate it. I fixed the specs, and the PR is ready for another review. Thanks!

randalldfloyd commented 3 weeks ago

@abelemlih @dlpierce

Just another FYI... I have ran this locally as part of other work I'm doing, and the sirenia app runs using fcrepo:6.5-tomcat9 without breaking on startup in the admin set service. I can merge soon if no other reviewers want to ring in on it.

abelemlih commented 1 week ago

@dlpierce I rebased this PR and it is now ready to review. Let me know if you have any feedback, thanks!