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

Ensure integrity of columns with foreign key constraints applied #973

Closed
Tracked by #451
kgodey opened this issue Jan 12, 2022 · 10 comments
Closed
Tracked by #451

Ensure integrity of columns with foreign key constraints applied #973

kgodey opened this issue Jan 12, 2022 · 10 comments
Labels
ready Ready for implementation type: bug Something isn't working work: backend Related to Python, Django, and simple SQL
Milestone

Comments

@kgodey
Copy link
Contributor

kgodey commented Jan 12, 2022

Problem

Users may try to change the data type of columns with foreign key constraints applied, which may break the foreign key.

Solution

The API should not allow the data type of a column with a foreign key constraint to be changed.

@kgodey kgodey changed the title We should not allow the data type of existing column with a foreign key constraint to be changed. Ensure integrity of columns with foreign key constraints applied Jan 12, 2022
@kgodey kgodey added this to the [08] Working with Views milestone Jan 12, 2022
@kgodey kgodey added ready Ready for implementation type: bug Something isn't working work: backend Related to Python, Django, and simple SQL labels Jan 12, 2022
@silentninja
Copy link
Contributor

Is this meant to be a validation step?

@kgodey
Copy link
Contributor Author

kgodey commented Jan 13, 2022

Is this meant to be a validation step?

I'm not sure what you mean.

@silentninja
Copy link
Contributor

Is this meant to be a validation step?

I'm not sure what you mean.

If we change the datatype of a foreign key, I think it will cause an error, so I am not sure where this scenario would occur. Will it be a validation we do on the service layer before it gets to the database since the database won't allow this anyway

@mathemancer
Copy link
Contributor

mathemancer commented Jan 13, 2022

The API should not allow the data type of a column with a foreign key constraint to be changed.

It's not possible to break the link by altering column type. It's disallowed at the database level, and would return an error. There are some constrained situations where type alterations are allowed, but not in ways that create problems. For performance and flexibility, I suggest letting the user make the request, and presenting a "nice" version of the error if it's disallowed by the database. This would allow, for example, altering the foreign key column and the column it references to a BIGINT if there's danger of a rollover in a smooth way that doesn't risk breaking integrity. Otherwise, you need to drop the constraint, alter each column, then rebuild the constraint. This introduces the potential for corruption if not handled correctly (i.e., in a single transaction).

@kgodey
Copy link
Contributor Author

kgodey commented Jan 13, 2022

@silentninja @mathemancer This is just a requirement from the product side, I did not consider whether the database allowed this or not when creating this ticket. If the DB doesn't allow it, handling this should just be creating the proper exception and error code and showing it in the API.

@mathemancer
Copy link
Contributor

@kgodey Would you be okay with altering types whenever it wouldn't break the link? I.e., when it creates no error at the DB layer?

@kgodey
Copy link
Contributor Author

kgodey commented Jan 14, 2022

@mathemancer Yup, that's fine.

@kgodey kgodey modified the milestones: [09] Working with Views, [08] Links Between Tables Jan 18, 2022
@mathemancer
Copy link
Contributor

In that case, I suggest just using the default DB behavior and handling errors gracefully.

@kgodey kgodey modified the milestones: [08] Links between Tables, Cycle 2 Jun 1, 2022
@github-actions
Copy link

github-actions bot commented May 8, 2023

This issue has not been updated in 90 days and is being marked as stale.

@github-actions github-actions bot added the stale label May 8, 2023
@seancolsen seancolsen removed the stale label Dec 5, 2023
@kgodey
Copy link
Contributor Author

kgodey commented Feb 2, 2024

Closing this, too old, requirements are likely out of date. We can create a new issue if we need this functionality in the future.

@kgodey kgodey closed this as not planned Won't fix, can't repro, duplicate, stale Feb 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready Ready for implementation type: bug Something isn't working work: backend Related to Python, Django, and simple SQL
Projects
No open projects
Development

No branches or pull requests

4 participants