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

types generated classes for xs:choice elements do not enable the user to tell which one was set #920

Closed
bwo opened this issue Jan 19, 2024 · 11 comments · Fixed by #946
Closed

Comments

@bwo
Copy link

bwo commented Jan 19, 2024

(context: I'm using xsdata as an intermediate format to generate conforming xml documents from an xsd file. but I think this would be an issue regardless.)

Given a type like

<xs:complexType name="Foo">
  <xs:choice>
    <xs:element name="Bar" type="X"/>
    <xs:element name="Baz" type="Y"/>
  </xs:choice>
</xs:complexType>

supposingthatXandY` are both string types of various sorts, xsdata generates a single field like:

   bar_or_baz: None | str = field(
        default=None,
        metadata={
            "type": "Elements",
            "choices": (
                {
                    "name": "Bar",
                    "type": str,
                    "namespace": "urn:whatever",
                    "min_length": 1,
                    "max_length": 4,
                },
                {
                    "name": "Baz",
                    "type": str,
                    "namespace": "urn:whatever",
                    "min_length": 1,
                    "max_length": 35,
                },
            ),
        },
    )

If bar_or_baz is set to a string of length, say, three, there's no way to tell, as far as I can see, whether it's the Bar element or the Baz element. But this could be very important!

When actually parsing from a document, it is possible to differentiate, but that's because the generated types are not correct; bar_or_baz will have the type None | DerivedElement. That also seems problematic; you cannot call len() on such an instance, for example.

@tefra
Copy link
Owner

tefra commented Jan 20, 2024

xsdata has to mark the filed as optional because of this

TypeError will be raised if a field without a default value follows a field with a default value. This is true whether this occurs in a single class, or as a result of class inheritance.

You can enable kw_only if you are on python >= 3.10 which solves that issue

For the other part, yes compound fields with ambiguous types is problematic, the derived element was introduced as a way to at least handle it and pass the w3c tests suite. I know it's not a very elegant way and I am open to suggestions :)

Duplicate #913

@tefra
Copy link
Owner

tefra commented Jan 20, 2024

Hey @bwo one alternative (fairly easy to implement) would be for the generator to detect the ambiguous types and try to generate dataclasses for these simple types.

Something like this

   bar_or_baz: None | str = field(
        default=None,
        metadata={
            "type": "Elements",
            "choices": (
                {
                    "name": "Bar",
                    "type": BarChoice,
                    "namespace": "urn:whatever",
                },
                {
                    "name": "Baz",
                    "type": BazChoice,
                    "namespace": "urn:whatever",
                },
            ),
        },
    )

@dataclass
class BarChoice:
    value: str = field(metadata={"min_length": 1, "max_length": 4})
    
@dataclass
class BazChoice:
    value: str = field(metadata={"min_length": 1, "max_length": 35})

I think that would work, it will add a bit more overhead with dataclasses but I am out of ideas

@tefra
Copy link
Owner

tefra commented Jan 20, 2024

ping @skinkie, would the above solution be acceptable?

@skinkie
Copy link
Contributor

skinkie commented Jan 20, 2024

@tefra interesting approach, I think it certainly would make sense. Have you tested this with more complex types (not a string field) with the BarChoice and BazChoice then do some inheritance or would it become 'value' again?

@tefra
Copy link
Owner

tefra commented Jan 20, 2024

I haven't tested anything :), I think it would work for all primitive values (and unions of primitive values) and enumerations.

Inheritance you mean BarChoice(str) to extend primitive types? I don't think we can easily do that and maintain things like restrictions.

@skinkie
Copy link
Contributor

skinkie commented Jan 20, 2024

I haven't tested anything :), I think it would work for all primitive values (and unions of primitive values) and enumerations.

Inheritance you mean BarChoice(str) to extend primitive types? I don't think we can easily do that and maintain things like restrictions.

If if the type would just be a facade (via value) to get it entered in the tree, that could make sense.

Can you explain why just the BarChoice(MoreComplex) wouldn't work? It might well be that your proposed solution is better than my question.

@tefra
Copy link
Owner

tefra commented Jan 25, 2024

I am leaning towards the dataclass option to be honest. For ambiguous class types with can do @dataclass BarChoice(AmbiguousClass) this will work out of the box, with no additional code.

For ambiguous primitive types (int,str,float,..., enums) we can either do

@dataclass
class BarChoice:
    value: str

The above will also work out of the box, with no additional code in the serializers/parsers and also has the benefit that we can carry restrictions as well. Eventually I will implement patterns/lengths :)

The alternative is to subclassing the primitive types, but we can't carry the restrictions without new code.

class BarChoice(str):
    ...

@tefra tefra mentioned this issue Feb 18, 2024
4 tasks
@tefra
Copy link
Owner

tefra commented Feb 27, 2024

Hey @bwo @skinkie can you give #946 a try?

There were a few additional scenarios that we missed initially plus how we deal with circular references, but it should be complete, all w3c tests and samples pass.

The generator will avoid generating ambiguous choices and the xml context builder will now raise an error if there are ambiguous choices.

By the default the intermediate classes are nested, unless if the unnest config is enabled or if there are circular references.

I still have to figure if we can avoid generating duplicate classes...

@skinkie
Copy link
Contributor

skinkie commented Feb 27, 2024

@tefra for my VDV server these are the only changes between release and this branch. Is that expected?

--- vdv453.prev/vdv453_incl_454_v2_3_2b_ohne_siri.py	2024-02-27 21:15:17.091607784 +0100
+++ vdv453/vdv453_incl_454_v2_3_2b_ohne_siri.py	2024-02-27 21:18:18.611800510 +0100
@@ -54,17 +54,17 @@
             "choices": (
                 {
                     "name": "KanalID",
-                    "type": str,
+                    "type": Type["AndmeldungType.KanalId"],
                     "namespace": "",
                 },
                 {
                     "name": "MeldungsID",
-                    "type": str,
+                    "type": Type["AndmeldungType.MeldungsId"],
                     "namespace": "",
                 },
                 {
                     "name": "FormatID",
-                    "type": str,
+                    "type": Type["AndmeldungType.FormatId"],
                     "namespace": "",
                 },
                 {
@@ -87,6 +87,30 @@
             },
         )
 
+    @dataclass(kw_only=True)
+    class KanalId:
+        value: str = field(
+            metadata={
+                "required": True,
+            }
+        )
+
+    @dataclass(kw_only=True)
+    class MeldungsId:
+        value: str = field(
+            metadata={
+                "required": True,
+            }
+        )
+
+    @dataclass(kw_only=True)
+    class FormatId:
+        value: str = field(
+            metadata={
+                "required": True,
+            }
+        )
+
 
 @dataclass(kw_only=True)
 class AzblinienspezialtextLoeschenType:

@skinkie
Copy link
Contributor

skinkie commented Feb 27, 2024

NeTEx is lovely.

diff -ur netex.old/netex_framework/netex_reusable_components/netex_vehicle_type_version.py netex.new/netex_framework/netex_reusable_components/netex_vehicle_type_version.py
--- netex.old/netex_framework/netex_reusable_components/netex_vehicle_type_version.py   2024-02-27 21:35:08.863199129 +0100
+++ netex.new/netex_framework/netex_reusable_components/netex_vehicle_type_version.py   2024-02-27 21:33:17.783052184 +0100
@@ -1,6 +1,6 @@
 from dataclasses import dataclass, field
 from decimal import Decimal
-from typing import List, Optional, Union, Any
+from typing import List, Optional, Type, Union, Any
 from xsdata.models.datatype import XmlDate
 from netex.netex_framework.netex_responsibility.netex_relationship import (
     ContainmentAggregationStructure,
@@ -1252,21 +1252,24 @@
             "tokens": True,
         },
     )
-    fuel_type_or_type_of_fuel: Optional[FuelTypeEnumeration] = field(
+    fuel_type_or_type_of_fuel: Optional[
+        Union[
+            "TransportTypeVersionStructure.FuelType",
+            "TransportTypeVersionStructure.TypeOfFuel",
+        ]
+    ] = field(
         default=None,
         metadata={
             "type": "Elements",
             "choices": (
                 {
                     "name": "FuelType",
-                    "type": List[FuelTypeEnumeration],
+                    "type": Type["TransportTypeVersionStructure.FuelType"],
                     "namespace": "http://www.netex.org.uk/netex",
-                    "default_factory": list,
-                    "tokens": True,
                 },
                 {
                     "name": "TypeOfFuel",
-                    "type": FuelTypeEnumeration,
+                    "type": Type["TransportTypeVersionStructure.TypeOfFuel"],
                     "namespace": "http://www.netex.org.uk/netex",
                 },
             ),
@@ -1297,6 +1300,24 @@
         },
     )
 
+    @dataclass(kw_only=True)
+    class FuelType:
+        value: List[FuelTypeEnumeration] = field(
+            default_factory=list,
+            metadata={
+                "required": True,
+                "tokens": True,
+            },
+        )
+
+    @dataclass(kw_only=True)
+    class TypeOfFuel:
+        value: FuelTypeEnumeration = field(
+            metadata={
+                "required": True,
+            }
+        )
+
 
 @dataclass(kw_only=True)
 class TypeOfDriverPermit(TypeOfDriverPermitValueStructure):

@tefra tefra closed this as completed in #946 Mar 2, 2024
@tefra
Copy link
Owner

tefra commented Mar 2, 2024

Tough cookie to crack this one, there were a few more use cases, the fix is on main

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

Successfully merging a pull request may close this issue.

3 participants