Skip to content

Conversation

@AdamGlustein
Copy link
Collaborator

@AdamGlustein AdamGlustein commented Oct 10, 2025

Cleanup and a few fixes to #574, thanks @sim15 for the contribution!

@timkpaine timkpaine added the type: enhancement Issues and PRs related to improvements to existing features label Oct 10, 2025
Signed-off-by: Adam Glustein <[email protected]>
@ptomecek
Copy link
Collaborator

ptomecek commented Oct 27, 2025

Previously, when all fields on structs were optional, we hard-coded these two flags for pydantic validation

Can we only apply this logic when allow_unset=True?
I think that when allow_unset is False, pydantic should infer it correctly based on the declared type and the default value, though we should add a unit test.

@AdamGlustein
Copy link
Collaborator Author

Previously, when all fields on structs were optional, we hard-coded these two flags for pydantic validation

Can we only apply this logic when allow_unset=True? I think that when allow_unset is False, pydantic should infer it correctly based on the declared type and the default value, though we should add a unit test.

I tested out this approach and it doesn't validate correctly in the case of a required field with a default value.

class MyStrictStruct(csp.Struct, allow_unset=False):
            req_default_str: str = "default"
E       pydantic_core._pydantic_core.SchemaError: Error building "list" validator:
E         SchemaError: Error building "function-wrap" validator:
E         SchemaError: Error building "typed-dict" validator:
E         SchemaError: Field 'req_default_str': a required field cannot have a default value

@AdamGlustein AdamGlustein requested a review from ptomecek November 3, 2025 20:59
Signed-off-by: Adam Glustein <[email protected]>
template<typename T>
bool InputAdapter::consumeTick( const T & value )
{
if constexpr( CspType::Type::fromCType<T>::type == CspType::TypeTraits::STRUCT )
Copy link
Collaborator

Choose a reason for hiding this comment

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

is there a central place we can do this check on push rather than consume? Will be much harder to track down on consumption

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did not see such a place - we could add it to each adapter type (push, pull, pushpull and managers) separately in their pushTick impls, but I think it's cleaner to have the validation logic in one spot.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree, but my point stands that it would be a lot harder to tell where the error is coming from
Also unfortunate to vall validate on all input adapters, even python based ones which were already validated due to PyStruct creation in python

@ptomecek
Copy link
Collaborator

Previously, when all fields on structs were optional, we hard-coded these two flags for pydantic validation

Can we only apply this logic when allow_unset=True? I think that when allow_unset is False, pydantic should infer it correctly based on the declared type and the default value, though we should add a unit test.

I tested out this approach and it doesn't validate correctly in the case of a required field with a default value.

class MyStrictStruct(csp.Struct, allow_unset=False):
            req_default_str: str = "default"
E       pydantic_core._pydantic_core.SchemaError: Error building "list" validator:
E         SchemaError: Error building "function-wrap" validator:
E         SchemaError: Error building "typed-dict" validator:
E         SchemaError: Field 'req_default_str': a required field cannot have a default value

I think this just means that if there is a default value, we leave it as "optional" (as the user doesn't have to provide it), but if there isn't, then it's not. I guess I was wrong about pydantic inferring it. However, the behavior for non-defaulted fields will be different for the two types of Struct.

@AdamGlustein AdamGlustein marked this pull request as draft November 19, 2025 21:23
…ield is set to None in the struct's bitmask

Signed-off-by: Adam Glustein <[email protected]>
Signed-off-by: Adam Glustein <[email protected]>
@AdamGlustein AdamGlustein marked this pull request as ready for review November 21, 2025 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: enhancement Issues and PRs related to improvements to existing features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants