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

Make default catalogNumber uniqueness rule editable #5400

Open
wants to merge 4 commits into
base: production
Choose a base branch
from

Conversation

melton-jason
Copy link
Contributor

@melton-jason melton-jason commented Nov 14, 2024

Fixes #5229

Checklist

  • Self-review the PR after opening it to make sure the changes look good
    and self-explanatory (or properly documented)
  • Add automated tests
  • Add relevant issue to release milestone

Testing instructions

  • Go the SchemaConfig page for the CollectionObject table
  • Ensure the "CatalogNumber must be unique to Collection" Uniqueness Rule which used to be readonly is now editable
  • Modify or delete the Uniqueness Rule
  • Save a CollectionObject with a duplicate catalognumber in the collection

For Developers (and/or the technically inclined with local instances)

  • This should be tested in both MySQL and MariaDB

  • After running/reversing the 0003_catnum_constraint migration, check the indices/constrains at the database level

    • This can accomplished with SHOW INDEXES FROM collectionobject;, SHOW CREATE TABLE collectionobject;, or a more specific query like: SELECT * FROM information_schema.statistics WHERE table_schema=(SELECT DATABASE()) AND table_name='collectionobject' AND non_unique=0;
  • After running the 0003_catnum_constraint migration, open Specify 6 and ensure that the basic functionality of

  • The reverse side of the 0004_catnum_uniquerule migration should handle the case where the Uniqueness Rule doesn't exist (i.e., deleted) or has been modified and recreate it.

    • Try both of the following and verify the state of the Uniqueness Rules:
      • Adding another field to the rule and reversing the migration
      • Deleting the uniquenessrule and reversing the migration

@melton-jason melton-jason added this to the 7.9.10 milestone Nov 14, 2024
@melton-jason melton-jason requested review from a team November 14, 2024 20:24
@CarolineDenis CarolineDenis modified the milestones: 7.9.10, 7.9.9 Nov 15, 2024
Copy link
Contributor

@alesan99 alesan99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing instructions

  • Go the SchemaConfig page for the CollectionObject table
  • Ensure the "CatalogNumber must be unique to Collection" Uniqueness Rule which used to be readonly is now editable
  • Modify or delete the Uniqueness Rule
  • Save a CollectionObject with a duplicate catalognumber in the collection

image

Working as expected 👍👍👍

@alesan99 alesan99 requested a review from a team November 15, 2024 15:56
Copy link
Collaborator

@emenslin emenslin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing instructions

  • Go the SchemaConfig page for the CollectionObject table
  • Ensure the "CatalogNumber must be unique to Collection" Uniqueness Rule which used to be readonly is now editable
  • Modify or delete the Uniqueness Rule
  • Save a CollectionObject with a duplicate catalognumber in the collection

Looks good, can modify and delete and add the rule back as long as there aren't any duplicates!

SELECT stat.INDEX_NAME
FROM information_schema.statistics stat
WHERE stat.TABLE_SCHEMA = %s
AND stat.TABLE_NAME = 'collectionobject'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You took the complicated route! I hope there aren't many instances where it differs from our current database schema, but at least it's managed. Nice work!

@grantfitzsimmons
Copy link
Member

@specify/dev-testing Someone needs to test this in MySQL 5.7. Does anyone have it installed?

@melton-jason
Copy link
Contributor Author

@specify/dev-testing Someone needs to test this in MySQL 5.7. Does anyone have it installed?

I tested this using the MySQL 5.7 docker image and so no difference from MariaDB.

If anyone else wishes to test this, here is the service I used in the docker-compose.yml file:

  mysql:
    platform: linux/x86_64
    image: mysql:5.7
    restart: unless-stopped
    environment:
      MYSQL_DATABASE: "specify"
      MYSQL_USER: "master"
      MYSQL_PASSWORD: "master"
      MYSQL_ROOT_PASSWORD: "root"
    ports:
      - "127.0.0.1:3006:3306"
    expose:
      - "3306"
    volumes:
      - "database:/var/lib/mysql"

I am using an M1 macOS, and the image is not supported on the architecture. I have the container running on platform: linux/x86_64, which works, but is pretty slow due to the emulation.

@melton-jason
Copy link
Contributor Author

@specify/dev-testing

There is one question which probably needs answering and bears discussion (or at least can be noted) before this is merged.

Let's say that a user has applied these two migrations and has created CollectionObjects with duplicate catalog numbers.

If they were to reverse the 0003_catnum_constraint migration, it would fail: the constraint can not be re-applied because there are records which violate it.
I am not saying this is incorrect, just that the behavior should be noted so that documentation made, and that the behavior could be changed if desired.

I think there are three approaches we can take:

  • Keep the migration as is. If the user wants to reverse the migration, they will have to manage the CollectionObjects with duplicate catalog numbers
  • In the reverse migration, don't add the constraint back to the database
    • There would be no difference to a user of the application or the API: the application still has the uniqueness rule to handle enforcement.
  • Temporarily disable checking uniqueness constraints during the migration, add the constraint, then re-enable checking uniqueness constraints

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 📋Back Log
Development

Successfully merging this pull request may close these issues.

Remove Uniqueness Constraint for Catalog Number
6 participants