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

fix: move RenameDuplicateAttributes to before SanitizeAttributesDefaultValue in Steps.SANITIZE #806

Merged
merged 2 commits into from Jun 24, 2023

Conversation

tlambert03
Copy link
Contributor

📒 Description

This fixes #805, an issue with enums with conflicting slugs

🔗 What I've Done

Write a description of the steps taken to resolve the issue

💬 Comments

While this fixes the problem i described... i have no idea what else it will break, so letting the tests run, and we can discuss

🛫 Checklist

@tlambert03
Copy link
Contributor Author

yeah... breaks the expectations set in RenameDuplicateAttributesTests.test_process ... So, I guess my question would be whether this is an issue with the enum naming itself, or in the referencing of the enum in the class default?

for example (from #805):

from dataclasses import dataclass, field
from enum import Enum

class UnitsPower(Enum):
    MW = "MW"
    M_W_1 = "mW"


@dataclass
class LightSource:
    power_unit: UnitsPower = field(
        default=UnitsPower.M_W,
        metadata={
            "name": "PowerUnit",
            "type": "Attribute",
        }
    )

should I be trying to convert the enum UnitsPower.M_W_1 -> UnitsPower.M_W, or LightSource.power_unit default from UnitsPower.M_W -> UnitsPower.M_W_1?

@tlambert03
Copy link
Contributor Author

tlambert03 commented Jun 20, 2023

a little more debugging...

after transformer.py has called classes = self.analyze_classes(self.classes) The attrs for the class defining the enum have been deduped:

[
    Class(
        qname='UnitsPower',
        attrs=[
            Attr(
                tag='Enumeration',
                name='MW',
                local_name='MW',
                ...
            ),
            Attr(
                tag='Enumeration',
                name='mW_1',
                local_name='mW',

but the @enum value in the default of the referring attribute doesn't seem to have gotten the memo:

    Class(
        qname='LightSource',
        attrs=[
            Attr(
                tag='Attribute',
                name='PowerUnit',
                local_name='PowerUnit',
                default='@enum@UnitsPower::mW',

... edit
and this is because in the order of processing:

def process(self):
"""The hidden naive recipe of processing xsd models."""
self.process_classes(Steps.UNGROUP)
self.remove_groups()
self.process_classes(Steps.FLATTEN)
self.filter_classes()
self.process_classes(Steps.SANITIZE)
self.process_classes(Steps.RESOLVE)
self.process_classes(Steps.FINALIZE)
self.designate_classes()

the field defaults get applied during step Steps.SANITIZE, but the enum field renaming doesn't happen until the next step: Steps.RESOLVE

@sonarcloud
Copy link

sonarcloud bot commented Jun 20, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@tlambert03 tlambert03 changed the title fix: fix rename dupe attrs fix: move RenameDuplicateAttributes to before SanitizeAttributesDefaultValue in Steps.SANITIZE Jun 20, 2023
@tlambert03
Copy link
Contributor Author

moving RenameDuplicateAttributes to right before SanitizeAttributesDefaultValue seems to have fixed it!

tests are all passing but let me know if maybe I'm missing some unexpected side effect here.
(note @tefra that I had to change the expectation in the test to match the new order).

The code output by my test case is now correct:

class UnitsPower(Enum):
    MW = "MW"
    M_W_1 = "mW"


@dataclass
class LightSource:
    power_unit: UnitsPower = field(
        default=UnitsPower.M_W_1,
        metadata={
            "name": "PowerUnit",
            "type": "Attribute",
        }
    )

@codecov
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (11dbca8) 99.96% compared to head (041fa3b) 99.96%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #806   +/-   ##
=======================================
  Coverage   99.96%   99.96%           
=======================================
  Files         104      104           
  Lines        9244     9244           
  Branches     2067     2067           
=======================================
  Hits         9241     9241           
  Partials        3        3           
Impacted Files Coverage Δ
xsdata/codegen/container.py 100.00% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tlambert03
Copy link
Contributor Author

happy to add a test for this if you tell me at what level it should be tested (i.e. full fixture or low level unit)

Copy link
Owner

@tefra tefra left a comment

Choose a reason for hiding this comment

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

Excellent troubleshoot!!!

@tefra tefra merged commit c72c4cd into tefra:master Jun 24, 2023
18 checks passed
@tefra
Copy link
Owner

tefra commented Jun 24, 2023

12k w3c tests and the sample suite all checked out.

@tlambert03 tlambert03 deleted the fix-enum branch June 24, 2023 15:00
@tlambert03 tlambert03 mentioned this pull request Feb 19, 2024
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 this pull request may close these issues.

Conflicting Enum values leading to attribute error on reference
2 participants