Skip to content

django: implement DatabaseIntrospection.get_primary_key_column() and add get_relations() stub #320

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

Merged
merged 1 commit into from
Mar 13, 2020

Conversation

odeke-em
Copy link
Contributor

@odeke-em odeke-em commented Mar 10, 2020

Implemented:

  • DatabaseIntrospection.get_primary_key_column
  • DatabaseIntrospection.get_relations

However, get_relations tests CANNOT yet be enabled because
Cloud Spanner doesn't yet have ON DELETE cascading for
FOREIGN KEYS which limits #313, which renders the method
useless as it will always return {} instead of being populated
with values that are derived from FOREIGN KEYs.

Fixes #312
Updates #311

@odeke-em odeke-em requested a review from timgraham March 10, 2020 22:43
@odeke-em odeke-em changed the title django/DatabaseIntrospection: implement get_primary_key_column, get_r… django/DatabaseIntrospection: implement get_primary_key_column, get_relations Mar 10, 2020
@timgraham timgraham changed the title django/DatabaseIntrospection: implement get_primary_key_column, get_relations django: implement DatabaseIntrospection.get_primary_key_column() and add get_relations() stub Mar 11, 2020
@odeke-em odeke-em force-pushed the get_table_relations+get_primary_key_columns branch from 65c6168 to 6a80eb3 Compare March 12, 2020 17:58
@@ -52,3 +52,48 @@ def get_table_description(self, cursor, table_name):
)

return descriptions

def get_relations(self, cursor, table_name):
# TODO: PLEASE DO NOT USE THIS METHOD UNTIL
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a slight preference for omitting this implementation for now (and instead return an empty dict) and add this code as part of #313 so the test can be enabled at the same time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem though is that digging up that code will require a context switch back and given the amount of time it took for me to dig through those docs I'd rather have it in here but with that warning, and not risk mistakenly going back to investigate again tediously. It'll return {} anyways.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think either of us would forget. Just put the method in a comment on #313. Going to have to edit the comment anyway. Your choice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the input! Given the amount of time that I spent going back into re-investigating the DATE->TIMESTAMP conversions, I am now wary about my ability to recover code that had been previously posted up, so for that I shall just merge this as is.

…elations

Implemented:
* DatabaseIntrospection.get_primary_key_column
* DatabaseIntrospection.get_relations

However, get_relations tests CANNOT yet be enabled because
Cloud Spanner doesn't yet have ON DELETE cascading for
FOREIGN KEYS which limits #313, which renders the method
useless as it will always return {} instead of being populated
with values that are derived from FOREIGN KEYs.

Fixes #312
Updates #311
@odeke-em odeke-em force-pushed the get_table_relations+get_primary_key_columns branch from 6a80eb3 to 9ca5262 Compare March 13, 2020 17:01
@odeke-em odeke-em requested a review from timgraham March 13, 2020 21:10
@@ -52,3 +52,48 @@ def get_table_description(self, cursor, table_name):
)

return descriptions

def get_relations(self, cursor, table_name):
# TODO: PLEASE DO NOT USE THIS METHOD UNTIL
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think either of us would forget. Just put the method in a comment on #313. Going to have to edit the comment anyway. Your choice.

@odeke-em odeke-em merged commit 9ca5262 into master Mar 13, 2020
@odeke-em
Copy link
Contributor Author

Thank you for the review @timgraham!

@odeke-em odeke-em deleted the get_table_relations+get_primary_key_columns branch March 13, 2020 21:55
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.

django: implement DatabaseIntrospection.get_primary_key_column()
2 participants