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

301 ngsi ld validate nested properties dynamically #320

Merged
merged 17 commits into from
Oct 29, 2024

Conversation

Maghnie
Copy link
Contributor

@Maghnie Maghnie commented Sep 3, 2024

Properties are now checked dynamically/recursively.

But list properties appear to cause errors:

======================================================================
ERROR: test_validate_subproperties_list (tests.models.test_ngsi_ld_context.TestLDContextModels)
Test the validation of multi-level properties in entities
----------------------------------------------------------------------
Traceback (most recent call last):
  File "d:\Repos\04-OpenSource\FiLiP\tests\models\test_ngsi_ld_context.py", line 277, in test_validate_subproperties_list
    entity4 = ContextLDEntity(**self.entity_list_property)
  File "d:\Repos\04-OpenSource\FiLiP\filip\models\ngsi_ld\context.py", line 503, in __init__
    data.update(self._validate_attributes(data))
  File "d:\Repos\04-OpenSource\FiLiP\filip\models\ngsi_ld\context.py", line 523, in _validate_attributes
    attrs[key] = ContextProperty.model_validate(attr)
  File "C:\Users\mmg\AppData\Local\anaconda3\envs\filip\lib\site-packages\pydantic\main.py", line 503, in model_validate
    return cls.__pydantic_validator__.validate_python(
pydantic_core._pydantic_core.ValidationError: 1 validation error for ContextProperty
  Input should be a valid dictionary or instance of ContextProperty [type=model_type, input_value=[{'type': 'Property', 'va...erty:gpsBxyz123-speed'}], input_type=list]
    For further information visit https://errors.pydantic.dev/2.5/v/model_type

Subproperties are now checked recursively to deal with nested properties. But list properties apparently cause errors
@djs0109
Copy link
Contributor

djs0109 commented Sep 5, 2024

As discussed yesterday, we would like to first not support the list property, coming from different sources

@Maghnie
Copy link
Contributor Author

Maghnie commented Sep 14, 2024

The recursive validation needed a fix, which is done now.

I added two new unit tests for testing the validation of subproperties. One test uses an entity dict with valid subproperties and another uses an invalid dict:
image

Both validations pass successfully. However, for the invalid dict, the wrong property declarations are automatically "fixed" by filip:

image

The question here is, whether this behavior is suitable, or is it better to simply reject the entity creation and request users to fix the property declarations themselves. If we're going with option 1, then this issue can be resolved.

@Maghnie Maghnie marked this pull request as ready for review September 14, 2024 07:59
@djs0109
Copy link
Contributor

djs0109 commented Sep 17, 2024

@Maghnie looks good! Regarding the preferred behavior you mention, option 2 makes more sense to me. Otherwise, users might keep the error in their script or data.

@djs0109 djs0109 changed the base branch from 221-ngsi-ld-migrate-V2-datamodels-in-ld to 212-testcase-for-every-endpoint-1 September 17, 2024 09:40
@djs0109
Copy link
Contributor

djs0109 commented Sep 17, 2024

@Maghnie right now _validate_single_property still have some problem when validating GeoProperty, because under GeoProperty there are normally some types, that are not in the white list

@djs0109 djs0109 added the NGSI-LD label Oct 8, 2024
…validate-nested-properties-dynamically

# Conflicts:
#	filip/models/ngsi_ld/context.py
@djs0109
Copy link
Contributor

djs0109 commented Oct 22, 2024

@Maghnie Could you look into this PR again? I have merged a branch and changed quit a lot. If that looks fine to you, I will merge and close this PR 🙂

@Maghnie
Copy link
Contributor Author

Maghnie commented Oct 22, 2024

@djs0109 Thanks for taking care of the issue during my vacation :)

The changes look good to me! especially the part with not having to check for geoproperty first :D

@djs0109 djs0109 changed the base branch from 212-testcase-for-every-endpoint-1 to NGSI-LD October 22, 2024 15:54
@djs0109 djs0109 merged commit 34355bf into NGSI-LD Oct 29, 2024
1 check failed
@djs0109 djs0109 deleted the 301-ngsi-ld-validate-nested-properties-dynamically branch October 29, 2024 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants