fix: Fix bug parsing allOf in nested keys BNCH-20174

[forest-benchling]

Description

Our code was not correctly generating models where allOf was done on a nested key, such as NotFoundError:

    NotFoundError:
      type: object
      additionalProperties: false
      properties:
        error:
          allOf:
            - $ref: '#/components/schemas/BaseError'
            - properties:
                invalidId:
                  type: string
                type:
                  type: string
                  enum:
                    - invalid_request_error

The generated model has no error property.

This bug also affects ContainerContent, Folder, AssayRunSchema, AssayRequestSchema, and potentially more.

This PR tries to fix the bug by changing _property_from_data and build_model_property.

Test Plan

• poetry run task check
• poetry run task re
• Made the changes in the openapi-specs submodule and rebuilt the client. Got these classes:

@attr.s(auto_attribs=True)
class NotFoundError:
    """  """

    error: Union[NotFoundErrorError, Unset] = UNSET
	...

@attr.s(auto_attribs=True)
class NotFoundErrorError:
    """  """

    invalid_id: Union[Unset, str] = UNSET
    type: Union[Unset, NotFoundErrorErrorType] = UNSET
    message: Union[Unset, str] = UNSET
    user_message: Union[Unset, str] = UNSET
    additional_properties: Dict[str, Any] = attr.ib(init=False, factory=dict)
	...

class NotFoundErrorErrorType(str, Enum):
    INVALID_REQUEST_ERROR = "invalid_request_error"

• Added automated test

[forest-benchling]

cc @bowenwr

[bowenwr]

@forest-benchling In scope of this how would you feel about fixing another issue with allOf that is blocking moving forward with the SDK?

Specifically that given this parameter in AaSequenceUpdate definition:

        fields:
          allOf:
            - $ref: "#/components/schemas/Fields"
            - description: >
                Fields to set on the AA sequence. Must correspond with the schema's field definitions.
                Every field should have its name as a key, mapping to an object with information about the value of
                the field.

We generate a field definition like:

    fields: Union[Unset, AaSequenceBaseRequestFields] = UNSET

Whereas we want to generate:

    fields: Union[Unset, Fields] = UNSET

The key to this is setting additionalProperties: false which would ideally be part of the allOf definition but setting it there has no effect in our current implementations.

If we corrected the spec to read:

    AaSequenceUpdate:
      allOf:
        - "$ref": "#/components/schemas/AaSequenceBaseRequest"
        - "$ref": "#/components/schemas/AaSequenceRequestAuthorIds"
        - "$ref": "#/components/schemas/AaSequenceRequestRegistryFields"
        - additionalProperties: false

I'd want it to work as specified above.

[packyg]

LGMT, though you should revert poetry.lock - we should keep in line with upstream dependencies.

[packyg]

@bowenwr it seems like you are getting at 2 distinct issues:

• Using allOf to tack on a description to an existing model
  • I think this would be do-able, but would need to think what the code would look like. Something like if we encounter an allOf and one element is a reference and the other(s) are inlined schemas that only have a description and/or example, we drop the inlined schemas and pull the reference up as if it was used directly
• Adding additionalProperties support to allOf

I think the first point is less critical - we can comment out the description part for now, use the $ref without the allOf, and circle back to this before we start using the specs for documentation.

Either way, we should keep this PR's scope as-is and follow up in a separate PR for any more changes.

[packyg]

After we merge this, we may want to do some massaging of the git history and squash this into the original allOf commit so there's just one commit for this feature, making it easier to keep the fork in sync with upstream.

Or we could do that next time we rebase from upstream and cut a main-v.0.7.4 (or whatever)

[forest-benchling]

Currently fails with

>       assert sorted(p.name for p in key_property.required_properties) == ["DateTime", "Float", "String"]
E       AssertionError: assert ['Float'] == ['DateTime', ...at', 'String']
E         At index 0 diff: 'Float' != 'DateTime'
E         Right contains 2 more items, first extra item: 'Float'
E         Use -v to get the full diff

I can't figure out why.

[packyg]

@forest-benchling I think this is because you only resolved references on the parent model, not on the key_property.

This should work if you do

Suggested change

	assert sorted(p.name for p in key_property.required_properties) == ["DateTime", "Float", "String"]
	assert isinstance(key_property, ModelProperty)
	key_property.resolve_references(components, schemas_holder)
	assert sorted(p.name for p in key_property.required_properties) == ["DateTime", "Float", "String"]

When you actually run the code generator, all models are resolved in build_schemas but we don't use that code path here.

[forest-benchling]

wooo thanks @packyg

[dtkav]

Hmm.. I remember this being important for some reason..
I think removing this might inlines all references rather than referencing them as objects.
You might have more context at this point though.

[forest-benchling]

@dtkav Can you clarify what you mean that it might inline all references?

[dtkav]

I generated the benchling client with both branches, and I think this is actually fine.
I was just trying (but failing) to remember the context around the "resolved later" comment.

It look like your PR generates 40 additional models which seem to correspond to actual missing functionality. 👍

[packyg]

There was a refactor upstream which I adapted the initial allOf implementation for. I think I shouldn't have included this there.
In that refactor, he changed the way property_from_data works such that it resolves references to models it has already built instead of returning a RefProperty that gets resolved later.

[dtkav]

👍