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

feat: Add allOf support for model definitions BNCH-18722 #12

Merged
merged 5 commits into from
Dec 8, 2020

Conversation

packyg
Copy link

@packyg packyg commented Dec 5, 2020

Collapses the child elements into one, without class heirarchy, mixins, etc

This is a replaying of 2670d11 (first implementation) and 9f5b95a (a bugfix) onto the new main-v.0.7.0, modified for the refactored upstream.

This should bring main-v.0.7.0 up to par with main for the features we implemented in our fork (dropping our Unset implementation for theirs)


We cannot really cut over to the upstream until a couple of issues are addressed:

See also: openapi-generators#98 (upstream issue by @dtkav) that this implements - we should try upstreaming this if all goes well

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Combines the child elements into one, without class heirarchy, mixins, etc

Verified

This commit was signed with the committer’s verified signature.
chiragsalian Chirag Chandrakant Salian

Unverified

This user has not yet uploaded their public signing key.
@packyg packyg requested review from dtkav, bowenwr and adamrp December 5, 2020 00:27
@packyg
Copy link
Author

packyg commented Dec 5, 2020

@adamrp is a non-blocking reviewer - just spreading context on the codegen/SDK

Copy link

@bowenwr bowenwr left a comment

Choose a reason for hiding this comment

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

What a journey. Thanks for all your efforts on this @packyg.

I generated a client with this and tried running a subset of our integration tests and they fail. In general, this PR looks OK to me but in 296ffb6 we introduce something like if not isinstance(some_prop, Unset): which is causing an error.

For example in list_boxes.py generated from this:

    json_sort: Union[Unset, ListBoxesSort] = UNSET
    if not isinstance(sort, Unset):
        json_sort = sort

Causes TypeError: isinstance() arg 2 must be a type or tuple of types. I think we need to be using sort == UNSET.

Something to visit next week as we close in on syncing everything up.

Copy link

@dtkav dtkav left a comment

Choose a reason for hiding this comment

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

Nice one!

Comment on lines +64 to +66
new_optional_props = [op for op in self.optional_properties if op.name != prop.name]
self.optional_properties.clear()
self.optional_properties.extend(new_optional_props)
Copy link

Choose a reason for hiding this comment

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

out of curiosity, what's the difference between this and

self.optional_properties = [op for op in self.optional_properties if op.name != prop.name]

?

Copy link
Author

Choose a reason for hiding this comment

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

@dtkav There isn't, except this class is marked with @attr.s(auto_attribs=True, frozen=True) so errors if I attempt to set the property

Copy link

Choose a reason for hiding this comment

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

Ah, TIL - thanks!

@packyg
Copy link
Author

packyg commented Dec 7, 2020

Causes TypeError: isinstance() arg 2 must be a type or tuple of types. I think we need to be using sort == UNSET.

Something to visit next week as we close in on syncing everything up.

Hmm I'll need to look at this more. The upstream is using the isinstance pattern and it seems to be working for them

@adamrp
Copy link

adamrp commented Dec 7, 2020

Causes TypeError: isinstance() arg 2 must be a type or tuple of types. I think we need to be using sort == UNSET.
Something to visit next week as we close in on syncing everything up.

Hmm I'll need to look at this more. The upstream is using the isinstance pattern and it seems to be working for them

Don't quote me on this, but I think isinstance has to take Python types (not Typing types) -- e.g., a class name or a tuple of class names

@packyg
Copy link
Author

packyg commented Dec 7, 2020

Don't quote me on this, but I think isinstance has to take Python types (not Typing types) -- e.g., a class name or a tuple of class names

That's what I don't understand - Unset is a class, which UNSET is an instance of

Copy link

@adamrp adamrp left a comment

Choose a reason for hiding this comment

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

Approving, although I wish I knew why isinstance is failing... 🤔

@bowenwr
Copy link

bowenwr commented Dec 7, 2020

Approving, although I wish I knew why isinstance is failing... 🤔

In my example it's because sort is passed as None which causes {NameError}name 'Unset' is not defined.

The generated client is looking for Union[Unset, ListBoxesSort]. We could change all the defaults in the SDK to use UNSET but I hate that it then exposes a pretty specific implementation detail.

Maybe we could persuade the maintainers to also check is not None but more likely I guess we could map None -> Unset for our cases where needed.

@packyg
Copy link
Author

packyg commented Dec 7, 2020

Maybe we could persuade the maintainers to also check is not None but more likely I guess we could map None -> Unset for our cases where needed.

@bowenwr We don't want them to do that, at least for JSON serialization, do we? If we did that it would be treating Unset the same as None and we're back to not having a distinction between unset/semantically meaningful None

As for URL and QS parameters -that's a different story, I could see the use in treating UNSET and None the same.


Still don't know why checking isinstance(None, Unset) causes a NameError - did Unset not get imported?


As far as for now - I think it'd be OK for us to use this implementation detail in the SDK since it's so tightly coupled to the generated client.

@bowenwr
Copy link

bowenwr commented Dec 7, 2020

Maybe we could persuade the maintainers to also check is not None but more likely I guess we could map None -> Unset for our cases where needed.

@bowenwr We don't want them to do that, at least for JSON serialization, do we? If we did that it would be treating Unset the same as None and we're back to not having a distinction between unset/semantically meaningful None

As for URL and QS parameters -that's a different story, I could see the use in treating UNSET and None the same.

Still don't know why checking isinstance(None, Unset) causes a NameError - did Unset not get imported?

As far as for now - I think it'd be OK for us to use this implementation detail in the SDK since it's so tightly coupled to the generated client.

@packyg

As for URL and QS parameters -that's a different story, I could see the use in treating UNSET and None the same.

Yes, this is the case to which I'm referring. Specifically a query param. For JSON serialization, you're correct.

did Unset not get imported?

It's imported.

Generated endpoint:

from typing import Any, Dict, Optional, Union

import httpx

from ...client import Client
from ...models.bad_request_error import BadRequestError
from ...models.box_list import BoxList
from ...models.list_boxes_sort import ListBoxesSort
from ...types import UNSET, Response, Unset


def _get_kwargs(
    *,
    client: Client,
    page_size: Union[Unset, int] = 50,
    next_token: Union[Unset, str] = UNSET,
    sort: Union[Unset, ListBoxesSort] = ListBoxesSort.MODIFIEDATDESC,
    schema_id: Union[Unset, str] = UNSET,
    modified_at: Union[Unset, str] = UNSET,
    name: Union[Unset, str] = UNSET,
    name_includes: Union[Unset, str] = UNSET,
    empty_positions: Union[Unset, int] = UNSET,
    empty_positionsgte: Union[Unset, int] = UNSET,
    empty_positionsgt: Union[Unset, int] = UNSET,
    empty_positionslte: Union[Unset, int] = UNSET,
    empty_positionslt: Union[Unset, int] = UNSET,
    empty_containers: Union[Unset, int] = UNSET,
    empty_containersgte: Union[Unset, int] = UNSET,
    empty_containersgt: Union[Unset, int] = UNSET,
    empty_containerslte: Union[Unset, int] = UNSET,
    empty_containerslt: Union[Unset, int] = UNSET,
    ancestor_storage_id: Union[Unset, str] = UNSET,
    storage_contents_id: Union[Unset, str] = UNSET,
    storage_contents_ids: Union[Unset, str] = UNSET,
    archive_reason: Union[Unset, str] = UNSET,
    ids: Union[Unset, str] = UNSET,
    barcodes: Union[Unset, str] = UNSET,
) -> Dict[str, Any]:
    url = "{}/boxes".format(client.base_url)

    headers: Dict[str, Any] = client.get_headers()

    json_sort: Union[Unset, ListBoxesSort] = UNSET
    if not isinstance(sort, Unset):
        json_sort = sort

    params: Dict[str, Any] = {}
    if page_size is not UNSET:
        params["pageSize"] = page_size
    if next_token is not UNSET:
        params["nextToken"] = next_token
    if sort is not UNSET:
        params["sort"] = json_sort
    if schema_id is not UNSET:
        params["schemaId"] = schema_id
    if modified_at is not UNSET:
        params["modifiedAt"] = modified_at
    if name is not UNSET:
        params["name"] = name
    if name_includes is not UNSET:
        params["nameIncludes"] = name_includes
    if empty_positions is not UNSET:
        params["emptyPositions"] = empty_positions
    if empty_positionsgte is not UNSET:
        params["emptyPositions.gte"] = empty_positionsgte
    if empty_positionsgt is not UNSET:
        params["emptyPositions.gt"] = empty_positionsgt
    if empty_positionslte is not UNSET:
        params["emptyPositions.lte"] = empty_positionslte
    if empty_positionslt is not UNSET:
        params["emptyPositions.lt"] = empty_positionslt
    if empty_containers is not UNSET:
        params["emptyContainers"] = empty_containers
    if empty_containersgte is not UNSET:
        params["emptyContainers.gte"] = empty_containersgte
    if empty_containersgt is not UNSET:
        params["emptyContainers.gt"] = empty_containersgt
    if empty_containerslte is not UNSET:
        params["emptyContainers.lte"] = empty_containerslte
    if empty_containerslt is not UNSET:
        params["emptyContainers.lt"] = empty_containerslt
    if ancestor_storage_id is not UNSET:
        params["ancestorStorageId"] = ancestor_storage_id
    if storage_contents_id is not UNSET:
        params["storageContentsId"] = storage_contents_id
    if storage_contents_ids is not UNSET:
        params["storageContentsIds"] = storage_contents_ids
    if archive_reason is not UNSET:
        params["archiveReason"] = archive_reason
    if ids is not UNSET:
        params["ids"] = ids
    if barcodes is not UNSET:
        params["barcodes"] = barcodes

    return {
        "url": url,
        "headers": headers,
        "cookies": client.get_cookies(),
        "timeout": client.get_timeout(),
        "params": params,
    }


def _parse_response(*, response: httpx.Response) -> Optional[Union[BoxList, BadRequestError]]:
    if response.status_code == 200:
        response_200 = BoxList.from_dict(response.json())

        return response_200
    if response.status_code == 400:
        response_400 = BadRequestError.from_dict(response.json())

        return response_400
    return None


def _build_response(*, response: httpx.Response) -> Response[Union[BoxList, BadRequestError]]:
    return Response(
        status_code=response.status_code,
        content=response.content,
        headers=response.headers,
        parsed=_parse_response(response=response),
    )


def sync_detailed(
    *,
    client: Client,
    page_size: Union[Unset, int] = 50,
    next_token: Union[Unset, str] = UNSET,
    sort: Union[Unset, ListBoxesSort] = ListBoxesSort.MODIFIEDATDESC,
    schema_id: Union[Unset, str] = UNSET,
    modified_at: Union[Unset, str] = UNSET,
    name: Union[Unset, str] = UNSET,
    name_includes: Union[Unset, str] = UNSET,
    empty_positions: Union[Unset, int] = UNSET,
    empty_positionsgte: Union[Unset, int] = UNSET,
    empty_positionsgt: Union[Unset, int] = UNSET,
    empty_positionslte: Union[Unset, int] = UNSET,
    empty_positionslt: Union[Unset, int] = UNSET,
    empty_containers: Union[Unset, int] = UNSET,
    empty_containersgte: Union[Unset, int] = UNSET,
    empty_containersgt: Union[Unset, int] = UNSET,
    empty_containerslte: Union[Unset, int] = UNSET,
    empty_containerslt: Union[Unset, int] = UNSET,
    ancestor_storage_id: Union[Unset, str] = UNSET,
    storage_contents_id: Union[Unset, str] = UNSET,
    storage_contents_ids: Union[Unset, str] = UNSET,
    archive_reason: Union[Unset, str] = UNSET,
    ids: Union[Unset, str] = UNSET,
    barcodes: Union[Unset, str] = UNSET,
) -> Response[Union[BoxList, BadRequestError]]:
    kwargs = _get_kwargs(
        client=client,
        page_size=page_size,
        next_token=next_token,
        sort=sort,
        schema_id=schema_id,
        modified_at=modified_at,
        name=name,
        name_includes=name_includes,
        empty_positions=empty_positions,
        empty_positionsgte=empty_positionsgte,
        empty_positionsgt=empty_positionsgt,
        empty_positionslte=empty_positionslte,
        empty_positionslt=empty_positionslt,
        empty_containers=empty_containers,
        empty_containersgte=empty_containersgte,
        empty_containersgt=empty_containersgt,
        empty_containerslte=empty_containerslte,
        empty_containerslt=empty_containerslt,
        ancestor_storage_id=ancestor_storage_id,
        storage_contents_id=storage_contents_id,
        storage_contents_ids=storage_contents_ids,
        archive_reason=archive_reason,
        ids=ids,
        barcodes=barcodes,
    )

    response = httpx.get(
        **kwargs,
    )

    return _build_response(response=response)


def sync(
    *,
    client: Client,
    page_size: Union[Unset, int] = 50,
    next_token: Union[Unset, str] = UNSET,
    sort: Union[Unset, ListBoxesSort] = ListBoxesSort.MODIFIEDATDESC,
    schema_id: Union[Unset, str] = UNSET,
    modified_at: Union[Unset, str] = UNSET,
    name: Union[Unset, str] = UNSET,
    name_includes: Union[Unset, str] = UNSET,
    empty_positions: Union[Unset, int] = UNSET,
    empty_positionsgte: Union[Unset, int] = UNSET,
    empty_positionsgt: Union[Unset, int] = UNSET,
    empty_positionslte: Union[Unset, int] = UNSET,
    empty_positionslt: Union[Unset, int] = UNSET,
    empty_containers: Union[Unset, int] = UNSET,
    empty_containersgte: Union[Unset, int] = UNSET,
    empty_containersgt: Union[Unset, int] = UNSET,
    empty_containerslte: Union[Unset, int] = UNSET,
    empty_containerslt: Union[Unset, int] = UNSET,
    ancestor_storage_id: Union[Unset, str] = UNSET,
    storage_contents_id: Union[Unset, str] = UNSET,
    storage_contents_ids: Union[Unset, str] = UNSET,
    archive_reason: Union[Unset, str] = UNSET,
    ids: Union[Unset, str] = UNSET,
    barcodes: Union[Unset, str] = UNSET,
) -> Optional[Union[BoxList, BadRequestError]]:
    """ List boxes """

    return sync_detailed(
        client=client,
        page_size=page_size,
        next_token=next_token,
        sort=sort,
        schema_id=schema_id,
        modified_at=modified_at,
        name=name,
        name_includes=name_includes,
        empty_positions=empty_positions,
        empty_positionsgte=empty_positionsgte,
        empty_positionsgt=empty_positionsgt,
        empty_positionslte=empty_positionslte,
        empty_positionslt=empty_positionslt,
        empty_containers=empty_containers,
        empty_containersgte=empty_containersgte,
        empty_containersgt=empty_containersgt,
        empty_containerslte=empty_containerslte,
        empty_containerslt=empty_containerslt,
        ancestor_storage_id=ancestor_storage_id,
        storage_contents_id=storage_contents_id,
        storage_contents_ids=storage_contents_ids,
        archive_reason=archive_reason,
        ids=ids,
        barcodes=barcodes,
    ).parsed


async def asyncio_detailed(
    *,
    client: Client,
    page_size: Union[Unset, int] = 50,
    next_token: Union[Unset, str] = UNSET,
    sort: Union[Unset, ListBoxesSort] = ListBoxesSort.MODIFIEDATDESC,
    schema_id: Union[Unset, str] = UNSET,
    modified_at: Union[Unset, str] = UNSET,
    name: Union[Unset, str] = UNSET,
    name_includes: Union[Unset, str] = UNSET,
    empty_positions: Union[Unset, int] = UNSET,
    empty_positionsgte: Union[Unset, int] = UNSET,
    empty_positionsgt: Union[Unset, int] = UNSET,
    empty_positionslte: Union[Unset, int] = UNSET,
    empty_positionslt: Union[Unset, int] = UNSET,
    empty_containers: Union[Unset, int] = UNSET,
    empty_containersgte: Union[Unset, int] = UNSET,
    empty_containersgt: Union[Unset, int] = UNSET,
    empty_containerslte: Union[Unset, int] = UNSET,
    empty_containerslt: Union[Unset, int] = UNSET,
    ancestor_storage_id: Union[Unset, str] = UNSET,
    storage_contents_id: Union[Unset, str] = UNSET,
    storage_contents_ids: Union[Unset, str] = UNSET,
    archive_reason: Union[Unset, str] = UNSET,
    ids: Union[Unset, str] = UNSET,
    barcodes: Union[Unset, str] = UNSET,
) -> Response[Union[BoxList, BadRequestError]]:
    kwargs = _get_kwargs(
        client=client,
        page_size=page_size,
        next_token=next_token,
        sort=sort,
        schema_id=schema_id,
        modified_at=modified_at,
        name=name,
        name_includes=name_includes,
        empty_positions=empty_positions,
        empty_positionsgte=empty_positionsgte,
        empty_positionsgt=empty_positionsgt,
        empty_positionslte=empty_positionslte,
        empty_positionslt=empty_positionslt,
        empty_containers=empty_containers,
        empty_containersgte=empty_containersgte,
        empty_containersgt=empty_containersgt,
        empty_containerslte=empty_containerslte,
        empty_containerslt=empty_containerslt,
        ancestor_storage_id=ancestor_storage_id,
        storage_contents_id=storage_contents_id,
        storage_contents_ids=storage_contents_ids,
        archive_reason=archive_reason,
        ids=ids,
        barcodes=barcodes,
    )

    async with httpx.AsyncClient() as _client:
        response = await _client.get(**kwargs)

    return _build_response(response=response)


async def asyncio(
    *,
    client: Client,
    page_size: Union[Unset, int] = 50,
    next_token: Union[Unset, str] = UNSET,
    sort: Union[Unset, ListBoxesSort] = ListBoxesSort.MODIFIEDATDESC,
    schema_id: Union[Unset, str] = UNSET,
    modified_at: Union[Unset, str] = UNSET,
    name: Union[Unset, str] = UNSET,
    name_includes: Union[Unset, str] = UNSET,
    empty_positions: Union[Unset, int] = UNSET,
    empty_positionsgte: Union[Unset, int] = UNSET,
    empty_positionsgt: Union[Unset, int] = UNSET,
    empty_positionslte: Union[Unset, int] = UNSET,
    empty_positionslt: Union[Unset, int] = UNSET,
    empty_containers: Union[Unset, int] = UNSET,
    empty_containersgte: Union[Unset, int] = UNSET,
    empty_containersgt: Union[Unset, int] = UNSET,
    empty_containerslte: Union[Unset, int] = UNSET,
    empty_containerslt: Union[Unset, int] = UNSET,
    ancestor_storage_id: Union[Unset, str] = UNSET,
    storage_contents_id: Union[Unset, str] = UNSET,
    storage_contents_ids: Union[Unset, str] = UNSET,
    archive_reason: Union[Unset, str] = UNSET,
    ids: Union[Unset, str] = UNSET,
    barcodes: Union[Unset, str] = UNSET,
) -> Optional[Union[BoxList, BadRequestError]]:
    """ List boxes """

    return (
        await asyncio_detailed(
            client=client,
            page_size=page_size,
            next_token=next_token,
            sort=sort,
            schema_id=schema_id,
            modified_at=modified_at,
            name=name,
            name_includes=name_includes,
            empty_positions=empty_positions,
            empty_positionsgte=empty_positionsgte,
            empty_positionsgt=empty_positionsgt,
            empty_positionslte=empty_positionslte,
            empty_positionslt=empty_positionslt,
            empty_containers=empty_containers,
            empty_containersgte=empty_containersgte,
            empty_containersgt=empty_containersgt,
            empty_containerslte=empty_containerslte,
            empty_containerslt=empty_containerslt,
            ancestor_storage_id=ancestor_storage_id,
            storage_contents_id=storage_contents_id,
            storage_contents_ids=storage_contents_ids,
            archive_reason=archive_reason,
            ids=ids,
            barcodes=barcodes,
        )
    ).parsed

@bowenwr
Copy link

bowenwr commented Dec 7, 2020

To be clear, the comments about Unset shouldn't hold up this PR. I just wanna capture it as part of our known "closing the gap" series.

@packyg packyg changed the base branch from main-v.0.7.0 to main-v.0.7.1 December 8, 2020 23:41
@packyg packyg changed the base branch from main-v.0.7.1 to main-v.0.7.0 December 8, 2020 23:41
@packyg packyg merged commit 782bbd0 into main-v.0.7.0 Dec 8, 2020
@packyg packyg deleted the update-allOf-support branch December 8, 2020 23:47
packyg added a commit that referenced this pull request Dec 8, 2020
Collapses the child elements into one, without class heirarchy, mixins, etc

This is a replaying of 2670d11 (first implementation) and 9f5b95a (a bugfix) onto the new `main-v.0.7.0`, modified for the refactored upstream.

This should bring `main-v.0.7.1` up to par with `main` for the features we implemented in our fork (dropping our `Unset` implementation for theirs)
packyg added a commit that referenced this pull request Dec 9, 2020
Collapses the child elements into one, without class heirarchy, mixins, etc

This is a replaying of 2670d11 (first implementation) and 9f5b95a (a bugfix) onto the new `main-v.0.7.0`, modified for the refactored upstream.

This should bring `main-v.0.7.1` up to par with `main` for the features we implemented in our fork (dropping our `Unset` implementation for theirs)
packyg added a commit that referenced this pull request Dec 9, 2020
Collapses the child elements into one, without class heirarchy, mixins, etc

This is a replaying of 2670d11 (first implementation) and 9f5b95a (a bugfix) onto the new `main-v.0.7.0`, modified for the refactored upstream.

This should bring `main-v.0.7.1` up to par with `main` for the features we implemented in our fork (dropping our `Unset` implementation for theirs)
bowenwr pushed a commit that referenced this pull request Jan 5, 2021
Collapses the child elements into one, without class heirarchy, mixins, etc

This is a replaying of 2670d11 (first implementation) and 9f5b95a (a bugfix) onto the new `main-v.0.7.0`, modified for the refactored upstream.

This should bring `main-v.0.7.1` up to par with `main` for the features we implemented in our fork (dropping our `Unset` implementation for theirs)
packyg added a commit that referenced this pull request Jan 13, 2021
Collapses the child elements into one, without class heirarchy, mixins, etc

This is a replaying of 2670d11 (first implementation) and 9f5b95a (a bugfix) onto the new `main-v.0.7.0`, modified for the refactored upstream.

This should bring `main-v.0.7.1` up to par with `main` for the features we implemented in our fork (dropping our `Unset` implementation for theirs)
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.

None yet

4 participants