Skip to content
This repository has been archived by the owner on Jun 28, 2024. It is now read-only.

feat: add required field validation #132

Closed
wants to merge 2 commits into from
Closed

Conversation

ctreatma
Copy link
Contributor

@ctreatma ctreatma commented Aug 28, 2023

This adds required field validation analogous to the required field validation that exists in the Java generator (TODO: link to example in metal-java).

Required fields are validated by:

  1. Creating a list of required property names
  2. Unmarshalling the current object into a map with string keys
  3. Validating that required property names appear as keys in the unmarshalled map

Templates are updated here: b5c00de
Code is regenerated here: 3e16fd3

@ctreatma ctreatma changed the title Custom templates feat: use custom templates in order to add required field validation Aug 28, 2023
@ctreatma
Copy link
Contributor Author

ctreatma commented Aug 28, 2023

Managing our own templates feels like too much, but I at least needed to do this in order to poke at options for validating the presence of required fields. Ideally this would be adopted upstream, but we may have to use custom templates at least for a little while.

It seems like we have to provide the full set of templates in order to modify just one or two, but I haven't confirmed that myself yet. We could also potentially keep the templates out of git (pull them out during CI, apply patches, and then generate code) but I feel like that's overly complicated.

metal/v1/model_address.go Outdated Show resolved Hide resolved
@ctreatma ctreatma force-pushed the custom_templates branch 2 times, most recently from 4123d0c to 64b83d5 Compare September 6, 2023 19:17
@ctreatma
Copy link
Contributor Author

ctreatma commented Sep 6, 2023

It seems like we have to provide the full set of templates in order to modify just one or two, but I haven't confirmed that myself yet. We could also potentially keep the templates out of git (pull them out during CI, apply patches, and then generate code) but I feel like that's overly complicated.

I was wrong about this; we can provide a subset of the templates. This PR has been updated to only include the templates that we are customizing in order to check for required fields. It's still a good practice to contribute template changes upstream whenever possible to avoid drift, though; custom templates mean we will miss out on upstream template changes unless we periodically re-sync and/or add yet another complicated patching process.

@ctreatma ctreatma force-pushed the custom_templates branch 2 times, most recently from e8d10d4 to 64fe14b Compare September 11, 2023 16:05
@ctreatma ctreatma force-pushed the custom_templates branch 2 times, most recently from 2888c0c to 1ef867e Compare September 14, 2023 20:42
@ctreatma ctreatma changed the title feat: use custom templates in order to add required field validation feat: add required field validation Sep 14, 2023
templates/.gitkeep Outdated Show resolved Hide resolved
@displague
Copy link
Member

We could also potentially keep the templates out of git (pull them out during CI, apply patches, and then generate code) but I feel like that's overly complicated.

That's what I was going to suggest. It's no more complicated that the existing patching and it makes it clear what changes we are making to the templates. It also makes it clear when we need to reconsider those changes in light up new upstream default templates which CI would stamp out before patching, as you suggest.

@ctreatma
Copy link
Contributor Author

ctreatma commented Nov 2, 2023

This is being implemented upstream in OpenAPITools/openapi-generator#16863

@ctreatma ctreatma closed this Nov 2, 2023
@ctreatma
Copy link
Contributor Author

This PR upgrades openapi-generator in order to get required field validation from upstream: #172

@ctreatma ctreatma deleted the custom_templates branch June 28, 2024 15:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants