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

Schema: Remove hardcoded enum values from customizable fields #9318

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

1337joe
Copy link
Contributor

@1337joe 1337joe commented Mar 17, 2025

In testing against the demo dataset I ran across a handful of enum types in the schema that accept custom values, which of course breaks schema validation because the custom values weren't enumerated at schema compile time.

I did not change the serializer behavior, I simply added extend_schema annotations to tell the schema builder to not treat them as enumerable choices.

Further work on #9045

Copy link

netlify bot commented Mar 17, 2025

Deploy Preview for inventree-web-pui-preview ready!

Name Link
🔨 Latest commit 1bb0f42
🔍 Latest deploy log https://app.netlify.com/sites/inventree-web-pui-preview/deploys/67d78d8a699dca00086cfb14
😎 Deploy Preview https://deploy-preview-9318--inventree-web-pui-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 97 (🟢 up 1 from production)
Accessibility: 89 (no change from production)
Best Practices: 100 (no change from production)
SEO: 78 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Mar 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.10%. Comparing base (9db5205) to head (6d05bdd).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #9318   +/-   ##
=======================================
  Coverage   86.09%   86.10%           
=======================================
  Files        1205     1205           
  Lines       52743    52757   +14     
  Branches     2258     2258           
=======================================
+ Hits        45410    45427   +17     
+ Misses       6751     6748    -3     
  Partials      582      582           
Flag Coverage Δ
backend 88.09% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@matmair
Copy link
Contributor

matmair commented Mar 17, 2025

Looking at the schema diff, we are losing a lot of information about formatting (currencies are always three letters capitalised, for example), sample values, etc.
Can this info be retained? Not everybody will generate a client - some will just look at the docs / swagger UI.

@1337joe
Copy link
Contributor Author

1337joe commented Mar 18, 2025

There are still enum status fields alongside the status_custom_key fields that I stripped the enums off of, but I agree it's less clear what the currency field accepts when it's just a string.

It looks like the label and help_text fields on the serializers pass through to the web interface, so the only way I can see to add more detail is to annotate in OpenApiExamples, which are at the operation level. I'll try to look into that when I have time.

@1337joe
Copy link
Contributor Author

1337joe commented Mar 24, 2025

@matmair Annotating examples onto the API everywhere currency is used hits a bunch of places (39 objects, so at least that many operations would need to be annotated), and annotated examples don't update with code changes so it would be a nightmare to maintain.

Since I can't change the label or help_text attached to the fields, the best solution I could come up with was to add a schema post-processor that checks for currency fields, then strips off the enum type while leaving the generated options list in the description. Would this be an acceptable solution (obviously requiring proper cleanup to fully back out the changes I made to Currency serialization instead of commenting them): 1337joe@0645e06

This results in the following schema changes as a sample (diff from master to the branch linked above):

         currency:
-          allOf:
-          - $ref: '#/components/schemas/SalePriceCurrencyEnum'
+          type: string
           description: |-
             Default currency used for this supplier

             * `AUD` - Australian Dollar
             * `CNY` - Chinese Yuan
             * `EUR` - Euro
             * `USD` - US Dollar

@matmair
Copy link
Contributor

matmair commented Apr 1, 2025

Have you checked if this can be achieved by changing the annotation of those fields? Maybe together with the schema generator we can find a more integrated approach.
The status fields already are custom fields so we might be able to leverage that too here. Maybe we just strip out the pattern at schema generation.

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.

2 participants