🚸 Auto populate fk constrained fields from Relationships
#94
Conversation
Codecov Report
@@ Coverage Diff @@
## main #94 +/- ##
==========================================
+ Coverage 86.64% 87.17% +0.52%
==========================================
Files 14 14
Lines 367 382 +15
==========================================
+ Hits 318 333 +15
Misses 49 49
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
lnschema_core/dev/sqlmodel.py
Outdated
| fk_columns = getattr(model.__class__, relationship_name).property.target.primary_key.columns._all_columns | ||
| fk_col_names = [column.name for column in fk_columns] | ||
| rel_field_name = relationship_name.replace("_", "") # this is delicate and relies on standards for naming fields | ||
| fk_fields = [(col_name, f"{rel_field_name}_{col_name}") for col_name in fk_col_names] |
There was a problem hiding this comment.
I don't fully understand why so much "post-processing" is needed including the flaky string replace after calling: getattr(model.__class__, relationship_name).property.target.primary_key.columns._all_columns
Is the complexity stemming from going from sa.Column to sqm.Field without there being a proper API for making that identification?
There was a problem hiding this comment.
There should be an API that directly gives you the column name corresponding to the relationship:
sqmrelationship.__sa_relationship__.foreign_key -> column/field
If there is a link table involved, there is no foreign_key, but instead, what sqlalchemy.relationship calls secondary table.
sqmrelationship.__sa_relationship__.secondary -> link table
There was a problem hiding this comment.
This is the correct UX!
The implementation gets more and more complicated, but it's probably still under control.
The code is super readable and well-commented. Great!
I'd fear that some more internals of dependent packages might break with this release. Not because this release is buggy, but because code has to be cleaned up in downstream packages.
Following up in more depth on the Slack convo: I fear we'll need to keep the non-standard sqm relationships which people will be able to set after calling the constructor. The most prominent example being DObject. Things like source should be set when constructing it. But linking Biosample against it should be possible post-construction.
On the other hand, all cases I have in mind might be safe anyway, because none of them will have a required foreign key field or required Relationship (and are implemented within non-type sqlalchemy).
No description provided.