Say what required the foreign key to resolve in naming conventions - #13499
Open
hamodywe wants to merge 1 commit into
Open
Say what required the foreign key to resolve in naming conventions#13499hamodywe wants to merge 1 commit into
hamodywe wants to merge 1 commit into
Conversation
A naming convention using %(referred_column_0_name)s reads fk.column while the constraint name is being built, which happens when the constraint is attached to its table. For a string-based ForeignKey that can be before the referenced Table exists in the MetaData -- every other path resolves the target lazily, so the same models work when the referenced table is declared first, or when the convention uses no referred_ token. The resulting NoReferencedTableError named the column and the missing table and said nothing about the naming convention, leaving nothing in the message to lead anyone to the cause. Re-raise it naming the token and the three ways out: declare the referenced table first, pass the Column object rather than a string, or use %(referred_table_name)s, which is read from the string and needs no resolution. The exception type and its table_name/column_name attributes are unchanged, so existing handling keeps working. Scoped to the error per the note on the issue that the naming mechanics are not expected to be easy to change here. Fixes: sqlalchemy#5350
Collaborator
|
Thanks! Issue #5350 is now marked code review in progress and no longer open for pull requests, so this pull request holds the review for it and another one won't land on top of your work. If this pull request is abandoned, a maintainer can put open for pull requests back on #5350 to reopen it to others. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes: #5350
Description
A naming convention using
%(referred_column_0_name)sreadsfk.columnwhile the constraint name is being built, which happens when the constraint is attached to its table. For a string-basedForeignKeythat can be before the referencedTableexists in theMetaData— every other path resolves the target lazily, which is why the same models work when the referenced table is declared first, or when the convention uses noreferred_token.Isolated on 2.1.0b4, with the child table declared first:
referred_*referred_column_0_nameNoReferencedTableErrorThe error named the column and the missing table and said nothing about the naming convention, so there was nothing in the message to lead anyone to the cause.
This re-raises it naming the token and the three ways out: declare the referenced table first, pass the
Columnobject rather than a string, or use%(referred_table_name)s, which is read from the string and needs no resolution. The exception type and itstable_name/column_nameattributes are unchanged, so existingexcept NoReferencedTableErrorhandling keeps working.Scoped to the error message per the note on the issue that the naming mechanics are not expected to be easy to change here.
Verification
naming.pyfailstest_fk_ref_unresolvable_target_reports_the_conventionwith the old bare messagetest/sql/+test/base/: 9037 passedtest/orm/declarative/: 2850 passedblack --checkandflake8(project config) cleanSupersedes #13498, which was auto-closed because its description was left as the template with an empty
Fixes:line.