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

to_dict breaks when a type is aliased and alias=True #338

Closed
criccomini opened this issue Jul 28, 2023 · 2 comments
Closed

to_dict breaks when a type is aliased and alias=True #338

criccomini opened this issue Jul 28, 2023 · 2 comments

Comments

@criccomini
Copy link
Contributor

criccomini commented Jul 28, 2023

Adding this param to the test_to_dict test"

(
    StructType(
        fields=[
            UnionType(
                types=[
                    NullType(),
                    StringType(bytes_=50),
                    IntType(bits=32),
                ],
                default=None,
                alias="recap.build.TestUnion",
            ),
        ],
    ),
    {
        "type": "struct",
        "fields": [
            {
                "type": "union",
                "alias": "recap.build.TestUnion",
                "types": [
                    {"type": "string", "bytes": 50},
                    "int32",
                ],
                "optional": True,
            }
        ],
    },
    True,
    True,
),

Fails with:

tests/unit/test_types.py:855: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
recap/types.py:498: in to_dict
    type_dict["fields"] = [
recap/types.py:499: in <listcomp>
    to_dict(
recap/types.py:531: in to_dict
    type_dict = alias_dict(type_dict) if alias else type_dict
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

type_dict = {'alias': 'recap.build.TestUnion', 'default': None, 'doc': None, 'logical': None, ...}
registry = <recap.types.RecapTypeRegistry object at 0x1265a60b0>

    def alias_dict(
        type_dict: dict[str, Any],
        registry: RecapTypeRegistry | None = None,
    ) -> dict[str, Any]:
        """
        Replaces concrete type definitions with aliases when possible.
    
        ```
        {
            "type": "int",
            "bits": 32,
            "signed": true,
        }
        ```
    
        Would become `{"type": "int32"}`.
    
        :param type_dict: A type dictionary.
        :param registry: A RecapTypeRegistry containing aliases.
        :return: A type dictionary with aliases.
        """
    
        registry = registry or RecapTypeRegistry()
    
        # If there's a matching alias, replace the type_dict with it
>       for alias, recap_type in registry._type_registry.items():
E       RuntimeError: dictionary changed size during iteration

I'm pretty sure it's because the UnionType is being added to the RecapTypeRegistry in from_dict (via alias_dict) while iteration is occurring.

@criccomini
Copy link
Contributor Author

Another test that breaks for test_to_dict:

(
    StructType(
        fields=[
            StringType(
                name="primary_phone",
                alias="build.recap.Phone",
                bytes_=10,
            ),
            UnionType(
                name="secondary_phone",
                types=[
                    NullType(),
                    ProxyType(
                        alias="build.recap.Phone",
                        registry=RecapTypeRegistry(),
                    ),
                ],
                default=None,
            ),
        ],
    ),
    {
        "type": "struct",
        "fields": [
            {
                "type": "string",
                "bytes": 10,
                "name": "primary_phone",
                "alias": "build.recap.Phone",
            },
            {
                "type": "build.recap.Phone",
                "name": "secondary_phone",
                "optional": True,
            },
        ],
    },
    True,
    True,
),

criccomini added a commit that referenced this issue Jul 29, 2023
The current Recap null-field handling is a bit cumbersome. I discussed with
@gwukelic, @adrianisk, and @cpard. We decided to add support for an `optional`
field to the CST. This will allow us to specify that a field is optional.

See the conversation here:

#335

Users can now specify an optional field as:

```yaml
type: struct
fields:
  - name: secondary_phone
    type: string32
    optional: true
```

This will get treated the same as:

```yaml
type: struct
fields:
  - name: secondary_phone
    type: "union",
    types: ["null", "string32"],
    default: null
```

I will update the Recap spec accordingly, as well.

Closes #335, #338

a
criccomini added a commit that referenced this issue Jul 29, 2023
The current Recap null-field handling is a bit cumbersome. I discussed with
@gwukelic, @adrianisk, and @cpard. We decided to add support for an `optional`
field to the CST. This will allow us to specify that a field is optional.

See the conversation here:

#335

Users can now specify an optional field as:

```yaml
type: struct
fields:
  - name: secondary_phone
    type: string32
    optional: true
```

This will get treated the same as:

```yaml
type: struct
fields:
  - name: secondary_phone
    type: "union",
    types: ["null", "string32"],
    default: null
```

I will update the Recap spec accordingly, as well.

Closes #335, #338
criccomini added a commit that referenced this issue Jul 30, 2023
The current Recap null-field handling is a bit cumbersome. I discussed with
@gwukelic, @adrianisk, and @cpard. We decided to add support for an `optional`
field to the CST. This will allow us to specify that a field is optional.

See the conversation here:

#335

Users can now specify an optional field as:

```yaml
type: struct
fields:
  - name: secondary_phone
    type: string32
    optional: true
```

This will get treated the same as:

```yaml
type: struct
fields:
  - name: secondary_phone
    type: "union",
    types: ["null", "string32"],
    default: null
```

I will update the Recap spec accordingly, as well.

Closes #335, #338
@criccomini
Copy link
Contributor Author

criccomini commented Jul 30, 2023

Closed by #340

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

1 participant