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

Fix uniqueness constraint with .auth.db.models.SqlUser.username #52

Merged
merged 1 commit into from
Nov 1, 2023

Conversation

amotl
Copy link
Contributor

@amotl amotl commented Nov 1, 2023

About

Just a minor followup to GH-46.

Trivia

The first CI run on this patch experienced an interesting fluke with CrateDB. We added a report at GH-53.

@amotl amotl force-pushed the mlflow-2.8 branch 2 times, most recently from 5f78a75 to 9da5b07 Compare November 1, 2023 14:58
Comment on lines 13 to 23
TODO: There are two more unique constraints defined on the MLflow data model.
- SqlExperimentPermission: "experiment_id", "user_id"
- SqlRegisteredModelPermission: "name", "user_id"
"""
from mlflow.store.model_registry.dbmodels.models import SqlRegisteredModel
from mlflow.store.tracking.dbmodels.models import SqlExperiment
from mlflow.server.auth.db.models import SqlUser

listen(SqlExperiment, "before_insert", check_uniqueness_factory(SqlExperiment, "name"))
listen(SqlRegisteredModel, "before_insert", check_uniqueness_factory(SqlRegisteredModel, "name"))
listen(SqlUser, "before_insert", check_uniqueness_factory(SqlUser, "username"))
Copy link
Contributor Author

@amotl amotl Nov 1, 2023

Choose a reason for hiding this comment

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

Other than the constraint on SqlUser added on behalf of this patch, there are two more to be addressed. Both can be found within *Permission types of classes. I don't know if you are using this subsystem of MLflow, @andnig?

Copy link
Contributor Author

@amotl amotl Nov 1, 2023

Choose a reason for hiding this comment

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

The emulative implementation currently is not able to handle composite unique key constraints, so I've created a corresponding issue to track it. Even more yak shaving ahead.

@amotl amotl marked this pull request as ready for review November 1, 2023 15:13
@amotl amotl marked this pull request as draft November 1, 2023 15:20
@amotl amotl marked this pull request as ready for review November 1, 2023 15:43
@amotl amotl requested a review from seut November 1, 2023 15:45
@amotl amotl merged commit a52633d into main Nov 1, 2023
1 check passed
@amotl amotl deleted the amo/fix-uniquness-SqlUser branch November 1, 2023 17:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants