-
Notifications
You must be signed in to change notification settings - Fork 36
🚸 Broaden name deduplication in HasType to search all contexts if no type is passed
#3850
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
base: main
Are you sure you want to change the base?
Changes from 7 commits
2c0725c
784951d
e8745e5
1b60822
4f7a15c
c92a784
bc43b13
b917409
03d36ca
6a01541
9033d3a
b5a25b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -610,7 +610,7 @@ def get_branch_id_for_create( | |
|
|
||
|
|
||
| def suggest_records_with_similar_names( | ||
| record: SQLRecord, name_field: str, kwargs | ||
| record: SQLRecord, name_field: str, kwargs, type_val=UNSET | ||
| ) -> SQLRecord | None: | ||
| """Returns a record if found exact match, otherwise None. | ||
|
|
||
|
|
@@ -624,10 +624,17 @@ def suggest_records_with_similar_names( | |
| # the below needs to be .first() because there might be multiple records with the same | ||
| # name field in case the record is versioned (e.g. for Transform key) | ||
| if isinstance(record, HasType): | ||
| if kwargs.get("type", None) is None: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would have expected something like this: type = kwargs.get("type", UNSET)
if type is UNSET:
subset = record.__class__.filter()
elif type is None:
subset = record.__class__.filter(type__isnull=True)
else:
subset = record.__class__.filter(type=kwargs["type"])
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a question whether we should use |
||
| subset = record.__class__.filter(type__isnull=True) | ||
| else: | ||
| subset = record.__class__.filter(type=kwargs["type"]) | ||
| if type_val is None: | ||
| # explicit type=None → always create new at root, skip dedup | ||
| if not kwargs.get("is_type", False): | ||
| logger.warning( | ||
| f"Creating a root-level {record.__class__.__name__.lower()} without" | ||
| " a type. In most cases, objects should be created under a type." | ||
| ) | ||
| return None | ||
| # UNSET: search all type contexts (catches typed records with same name, fixes silent dup bug) | ||
| # <object>: search within that type context only | ||
| subset = record.__class__ if type_val is UNSET else record.__class__.filter(type=type_val) | ||
| else: | ||
| subset = record.__class__ | ||
| exact_match = subset.filter(**{name_field: kwargs[name_field]}).first() | ||
|
|
@@ -1162,10 +1169,15 @@ class Meta: | |
|
|
||
| def __init__(self, *args, **kwargs): | ||
| skip_validation = kwargs.pop("_skip_validation", False) | ||
| # strip sentinel before validate_fields and Django's Model.__init__ see it | ||
| # capture user intent before stripping: UNSET=not passed, None=explicit None, obj=typed | ||
| # Schema pre-computes this (converts UNSET→None for hashing) and passes it via _type_val | ||
| # `is` never calls __eq__, so FeaturePredicate objects are safe | ||
| if isinstance(self, HasType) and kwargs.get("type", UNSET) is UNSET: | ||
| kwargs.pop("type", None) | ||
| if isinstance(self, HasType): | ||
| type_val = kwargs.pop("_type_val", kwargs.get("type", UNSET)) | ||
| if type_val is UNSET: | ||
| kwargs.pop("type", None) | ||
| else: | ||
| type_val = UNSET | ||
| if not args: | ||
|
|
||
| def resolve_fk_or_id(field_name: str) -> bool: | ||
|
|
@@ -1240,7 +1252,7 @@ def resolve_fk_or_id(field_name: str) -> bool: | |
| ): | ||
| name_field = getattr(self, "_name_field", "name") | ||
| exact_match = suggest_records_with_similar_names( | ||
| self, name_field, kwargs | ||
| self, name_field, kwargs, type_val | ||
| ) | ||
| if exact_match is not None: | ||
| if "version_tag" in kwargs: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -690,14 +690,17 @@ def test_feature_manager_raise_not_validated_values(): | |
| def test_name_lookup(): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aren't we now missing a test here where the user explicitly passes |
||
| my_type = ln.Record(name="MyType", is_type=True).save() | ||
| label1 = ln.Record(name="label 1", type=my_type).save() | ||
| # same type → returns existing typed record | ||
|
ishitajain9717 marked this conversation as resolved.
|
||
| label2 = ln.Record(name="label 1", type=my_type) | ||
| assert label2 == label1 | ||
| # no type passed, only typed record exists → fallback returns the typed one | ||
|
ishitajain9717 marked this conversation as resolved.
|
||
| label2 = ln.Record(name="label 1") | ||
| assert label2 != label1 | ||
| label2.save() | ||
| label3 = ln.Record(name="label 1") | ||
| assert label3 == label2 | ||
| label2.delete(permanent=True) | ||
| assert label2 == label1 | ||
| # root-level record exists → no-type search finds root first before typed | ||
| root_label = ln.Record(name="root label 1").save() | ||
| label3 = ln.Record(name="root label 1") | ||
| assert label3 == root_label | ||
| root_label.delete(permanent=True) | ||
| label1.delete(permanent=True) | ||
| my_type.delete(permanent=True) | ||
|
|
||
|
|
@@ -1194,7 +1197,7 @@ def test_record_features_add_remove_values(): | |
|
|
||
| # test passing ISO-format date string for date | ||
|
|
||
| test_record2 = ln.Record(name="test_record").save() | ||
| test_record2 = ln.Record(name="test_record", type=None).save() | ||
| # we could also test different ways of formatting but don't yet do that | ||
| # in to_dataframe() we enforce ISO format already | ||
| feature_date = ln.Feature.get(name="feature_date") | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to introduce
type_val? Thekwargshavetypein them and you can detect whether it matchesUNSET. Why is that not possible?