Skip to content

better support for merging properties with allOf #1096

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

Merged
merged 25 commits into from
Sep 7, 2024

Conversation

eli-bl
Copy link
Collaborator

@eli-bl eli-bl commented Aug 14, 2024

Fixes #1090 (with some limitations - see below)

This PR allows the generator to work with some valid schemas where the same property name is specified by more than one schema within an allOf, where previously it would have failed.

Background

As described in the issue, OpenAPI and JSON Schema define allOf in a fairly vague and tolerant way. If a schema is defined like this:

  C:
    allOf:
      - $ref: "/path/to/schema/A" 
      - $ref: "/path/to/schema/B"

—then the validation for C is "must pass validation for A, and for B". A and B are both objects, and they both have property X, then the value for X must pass validation for A.X and B.X; the validation is the strictest subset of both.

This does not map very cleanly to the client generator's internal model. Because its ultimate goal is to producing Python model classes with properties of specific types, we need to be able to "merge" A.X and B.X to produce a single instance of some ___Property class, whose attributes are derived from both.

The logic for doing this was narrower than necessary. If, for instance, A.X and B.X only differed in terms of their description, or if one of them specified a default value and the other did not, the generator would reject them as being too different.

Solution

This PR defines a more nuanced property merge logic, as follows:

  • If both versions of the property are of the same type, the result is a property of the same type. Its validation rules, if any, are the strictest subset of the validation rules for both (for instance, "string with max length 3" and "string with max length 5" produces "string with max length 3").
  • Int and number (aka float) properties can always be merged; the result is an int property (since "must be integer" is a stricter rule than "must be a number"). This is only invalid if there is a non-integer default value.
  • "Any" can be merged with any property type; the result is that property type (since "must be a ___" is stricter than "can be anything").
  • For common string-valued attributes such as description, earlier values are overridden by later values, on the assumption that the second schema is meant to be an extension or specialization of the first.

It also preserves helpful aspects of the original implementation:

  • Two enums can be merged, if the values for one are a subset of the other; the result uses the subset.
  • A string enum can be merged with a string, or an int enum with an int; the result is an enum.

Limitations

Due to limitations of our internal model, some combinations of properties that are valid in OpenAPI will still be rejected by this implementation:

  • Two string properties with different pattern attributes cannot be merged, even if there are values that could match both of those regexes.
  • Two list types that contain different schemas cannot be merged, even though logically this should mean "every element in the list must match both of those schemas".
  • OpenAPI allows minimum and maximum to be specified for numeric types, but (as far as I can tell) the generator currently ignores these altogether, so the merge logic doesn't take them into account either.

Compatibility

This PR preserves the previous schema parsing behavior in all cases where the generator would have previously succeeded. The new behaviors all apply to valid schemas where the generator would have previously failed.

@eli-bl
Copy link
Collaborator Author

eli-bl commented Aug 21, 2024

Whoops, I neglected to lint this and so I didn't see a bunch of type-hinting issues I'll need to fix. I understand why Property and PropertyProtocol have been defined as separate things, but that does make it fairly challenging to deal with code that manipulates the various property classes.

@eli-bl eli-bl marked this pull request as draft August 21, 2024 22:09
@eli-bl eli-bl marked this pull request as ready for review August 21, 2024 22:30
@eli-bl eli-bl force-pushed the issue-1090-merge-allof branch from 8f788bd to 02f6b68 Compare August 21, 2024 22:41
@eli-bl eli-bl force-pushed the issue-1090-merge-allof branch from 02f6b68 to 8722001 Compare August 21, 2024 22:41
@dbanty
Copy link
Collaborator

dbanty commented Aug 25, 2024

I changed a few things around here, notably:

  1. Stop checking max length and pattern, since we don't use/enforce them at all in the templates (as far as I remember / can tell). No reason to complain about them to users if we haven't implemented them anyway. I figure once we get around to pattern support for real, we can just make it a list of patterns and the code will verify it matches all?
  2. Validate & convert all default types. For example, if merging an enum with an int prop, and the int prop as a default which doesn't match an enum value, that's an error.
  3. Support merging arrays if the inner types can be merged

I think, before merging, we should:

  • Add some e2e tests so we try out generating some common merges. This would have caught us not converting raw values to enum variants, and might catch something else too
  • Allow merging a string with no format to a string with a format. Ideally, we'll combine that logic with _string_based_property somehow so if we add more formats in the future, they're also supported. Right now it's DateProperty, DateTimeProperty, and FileProperty

I suppose that last one could also be a separate follow-up, but I think it won't be too heavy of a lift?

@dbanty dbanty added the 🐞bug Something isn't working label Aug 25, 2024
@eli-bl
Copy link
Collaborator Author

eli-bl commented Aug 26, 2024

@dbanty Thanks for taking a look at this! I'm happy to take a stab at those last 2 changes you suggested.

(Edited: I had a question about your removal of the maxLength/pattern stuff, but never mind that, because I had missed the full extent of your changes - I thought you were only removing the logic for merging them, but really you removed them entirely from StringProperty. That makes more sense to me in terms of not having them in this "kind of implemented internally, but not really" state, although I do think it's a little unfortunate that it means breaking any custom template implementations people might have had, if they really wanted maxLength/pattern to work.)

@eli-bl eli-bl force-pushed the issue-1090-merge-allof branch from b60c1ba to 1e995b7 Compare September 3, 2024 18:33
@eli-bl eli-bl force-pushed the issue-1090-merge-allof branch from a3f7c33 to 6bdf1df Compare September 3, 2024 22:56
@eli-bl eli-bl force-pushed the issue-1090-merge-allof branch from 6bdf1df to 4046d03 Compare September 3, 2024 22:59
@eli-bl eli-bl requested a review from dbanty September 3, 2024 23:05
@dbanty dbanty changed the title fix: better support for merging properties with allOf better support for merging properties with allOf Sep 7, 2024
Copy link
Collaborator

@dbanty dbanty left a comment

Choose a reason for hiding this comment

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

Thanks so much for this one @eli-bl! I'll probably take a stab at refactoring the Value stuff elsewhere, but I don't want to keep adding scope to this PR.

@dbanty dbanty added this pull request to the merge queue Sep 7, 2024
Merged via the queue into openapi-generators:main with commit 0bdd8ba Sep 7, 2024
19 checks passed
@eli-bl eli-bl deleted the issue-1090-merge-allof branch September 9, 2024 16:56
micha91 pushed a commit to micha91/openapi-python-client that referenced this pull request May 13, 2025
…1096)

Fixes openapi-generators#1090 (with some limitations - see below)

This PR allows the generator to work with some valid schemas where the
same property name is specified by more than one schema within an
`allOf`, where previously it would have failed.

# Background

As described in the issue, OpenAPI and JSON Schema define `allOf` in a
fairly vague and tolerant way. If a schema is defined like this:
```yaml
  C:
    allOf:
      - $ref: "/path/to/schema/A" 
      - $ref: "/path/to/schema/B"
```

—then the validation for C is "must pass validation for A, and for B". A
and B are both objects, and they both have property X, then the value
for X must pass validation for A.X and B.X; the validation is the
strictest subset of both.

This does not map very cleanly to the client generator's internal model.
Because its ultimate goal is to producing Python model classes with
properties of specific types, we need to be able to "merge" A.X and B.X
to produce a single instance of some ___Property class, whose attributes
are derived from both.

The logic for doing this was narrower than necessary. If, for instance,
A.X and B.X only differed in terms of their `description`, or if one of
them specified a `default` value and the other did not, the generator
would reject them as being too different.

# Solution

This PR defines a more nuanced property merge logic, as follows:

- If both versions of the property are of the same type, the result is a
property of the same type. Its validation rules, if any, are the
strictest subset of the validation rules for both (for instance, "string
with max length 3" and "string with max length 5" produces "string with
max length 3").
- Int and number (aka float) properties can always be merged; the result
is an int property (since "must be integer" is a stricter rule than
"must be a number"). This is only invalid if there is a non-integer
default value.
- "Any" can be merged with any property type; the result is that
property type (since "must be a ___" is stricter than "can be
anything").
- For common string-valued attributes such as `description`, earlier
values are overridden by later values, on the assumption that the second
schema is meant to be an extension or specialization of the first.

It also preserves helpful aspects of the original implementation:

- Two enums can be merged, if the values for one are a subset of the
other; the result uses the subset.
- A string enum can be merged with a string, or an int enum with an int;
the result is an enum.

# Limitations

Due to limitations of our internal model, some combinations of
properties that are valid in OpenAPI will still be rejected by this
implementation:

- Two string properties with different `pattern` attributes cannot be
merged, even if there are values that could match both of those regexes.
- Two list types that contain different schemas cannot be merged, even
though logically this should mean "every element in the list must match
both of those schemas".
- OpenAPI allows `minimum` and `maximum` to be specified for numeric
types, but (as far as I can tell) the generator currently ignores these
altogether, so the merge logic doesn't take them into account either.

# Compatibility

This PR preserves the previous schema parsing behavior in all cases
where the generator would have previously succeeded. The new behaviors
all apply to valid schemas where the generator would have previously
failed.

---------

Co-authored-by: Dylan Anthony <[email protected]>
Co-authored-by: Dylan Anthony <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

too strict criteria for merging properties with allOf
2 participants