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

Use SQLAlchemy relationships instead of the foreign keys directly #116

Open
mprahl opened this issue Mar 30, 2020 · 2 comments
Open

Use SQLAlchemy relationships instead of the foreign keys directly #116

mprahl opened this issue Mar 30, 2020 · 2 comments

Comments

@mprahl
Copy link
Contributor

mprahl commented Mar 30, 2020

In models.py, there are a few places where the foreign key is directly used instead of a relationship. We should use a relationship when possible.

For example:
https://github.com/release-engineering/cachito/blob/b3a7c5a256f450cd16f419dbf4e420690ed8f533/cachito/web/models.py#L540-L543

This is another instance:
https://github.com/release-engineering/cachito/blob/b3a7c5a256f450cd16f419dbf4e420690ed8f533/cachito/web/models.py#L574

Investigate if its worth using an alternate join condition here instead of this method:
https://github.com/release-engineering/cachito/blob/b3a7c5a256f450cd16f419dbf4e420690ed8f533/cachito/web/models.py#L387-L391

This might be another potential area for improvement (needs investigation):
https://github.com/release-engineering/cachito/blob/b3a7c5a256f450cd16f419dbf4e420690ed8f533/cachito/web/models.py#L312-L349

This might be also another potential area for improvement (needs investigation):
https://github.com/release-engineering/cachito/blob/b3a7c5a256f450cd16f419dbf4e420690ed8f533/cachito/web/models.py#L351-L377

@tkdchen
Copy link
Contributor

tkdchen commented Apr 27, 2020

@mprahl I makde some changes to the code for this issue, however I can't figure out an improvement to dependencies_count and packages_count except changing the filter criterion like RequestDependency.request_id == self.id based on a relationship property. Any idea? What is your thought on the potential area for improvement originally?

@mprahl
Copy link
Contributor Author

mprahl commented Apr 29, 2020

@mprahl I makde some changes to the code for this issue, however I can't figure out an improvement to dependencies_count and packages_count except changing the filter criterion like RequestDependency.request_id == self.id based on a relationship property. Any idea? What is your thought on the potential area for improvement originally?

@tkdchen I think it'd be cleaner to use a relationship instead of the foreign keys directly in the filter as you mentioned, but it's fine to keep it as is. I'll leave the decision up to you if you'd like to change it.

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

No branches or pull requests

2 participants