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

Fixed #30396 -- Added system checks for uniqueness of indexes and constraints names. #11278

Merged
merged 1 commit into from May 2, 2019

Conversation

cansarigol
Copy link
Contributor

Ticket: #30396

Copy link
Member

@charettes charettes left a comment

Choose a reason for hiding this comment

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

Any thoughts about adjusting the message to make it clear the check is performed on all models and include them in the message like we did for E028.

e.g.

  • `'foo' index name is not unique amongst models: '
  • 'foo' index name is not unique for model <model> (when len(model list) == 1).

django/core/checks/model_checks.py Outdated Show resolved Hide resolved
django/core/checks/model_checks.py Outdated Show resolved Hide resolved
django/core/checks/model_checks.py Outdated Show resolved Hide resolved
django/core/checks/model_checks.py Outdated Show resolved Hide resolved
docs/ref/checks.txt Outdated Show resolved Hide resolved
django/core/checks/model_checks.py Outdated Show resolved Hide resolved
@cansarigol
Copy link
Contributor Author

thanks @charettes, I applied your comments.

Copy link
Member

@felixxm felixxm left a comment

Choose a reason for hiding this comment

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

@cansarigol Thanks for this patch. I left some comments.

index_names.append(model_index.name)
for model_constraint in model._meta.constraints:
constraint_models.append(model)
constraint_names.append(model_constraint.name)
Copy link
Member

Choose a reason for hiding this comment

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

I would use the same mechanism as for the E020 and models' labels instead of __name__'s, i.e.

indexes = defaultdict(list)
constraints = defaultdict(list)
...
for model_index in model._meta.indexes:
    indexes[model_index.name].append(model._meta.label)
for model_constraint in model._meta.constraints:
    constraints[model_constraint.name].append(model._meta.label)

django/core/checks/model_checks.py Outdated Show resolved Hide resolved
* **models.E029**: ``<index>`` index name is not unique amongst models:
``<model list>``.
* **models.E030**: ``<constraint>`` constraint name is not unique amongst models:
``<model list>``.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe:

* **models.E029**: index name ``<index>`` is not unique for model ``<model>``.
  ``<model list>``.
* **models.E030**: index name ``<index>`` is not unique amongst models:
  ``<model list>``.
* **models.E031**: constraint name ``<constraint>`` is not unique for model
  ``<model>``.
* **models.E032**: constraint name ``<constraint>`` is not unique amongst
  models: ``<model list>``.

@felixxm felixxm changed the title Fixed #30396 -- Added system check for uniqueness of partial indexes and constraints names. Fixed #30396 -- Added system checks for uniqueness of indexes and constraints names. May 1, 2019
@felixxm felixxm self-assigned this May 1, 2019
@cansarigol cansarigol force-pushed the ticket-30396 branch 2 times, most recently from f38c739 to 616b13d Compare May 1, 2019 13:39
@cansarigol
Copy link
Contributor Author

thanks, @felixxm, I changed the pr as your comments. Isn't Error.obj necessary?

@felixxm
Copy link
Member

felixxm commented May 1, 2019

@cansarigol obj is not necessary and in this case it doesn't have much value.

@cansarigol
Copy link
Contributor Author

@cansarigol obj is not necessary and in this case it doesn't have much value.

you're right. besides that, there are multiple objects in plural case.

@felixxm
Copy link
Member

felixxm commented May 2, 2019

I rebased from master, split tests, and added tests for collisions across different apps and from abstract models.

…straints names.

Co-Authored-By: Mariusz Felisiak <felisiak.mariusz@gmail.com>
@felixxm felixxm merged commit bceadd2 into django:master May 2, 2019
@cansarigol cansarigol deleted the ticket-30396 branch May 2, 2019 07:50
@cansarigol
Copy link
Contributor Author

the code is nicer and more inclusive in this way. thank you for your help @felixxm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants