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

Update schemas.py #2717

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Update schemas.py #2717

wants to merge 1 commit into from

Conversation

gansik
Copy link

@gansik gansik commented Oct 4, 2024

Overwriting an iterated variable inside a loop

Summary

The easiest way to describe the problem is to simply quote the code:

for route in routes:
    routes = route.routes or []
    ...

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

Sub routes rewrite parent routes
@Kludex
Copy link
Member

Kludex commented Oct 9, 2024

Thanks @gansik :)

Would you like to add a more meaningful PR title?

@gansik
Copy link
Author

gansik commented Oct 9, 2024

Would you like to add a more meaningful PR title?

I've added more information. This is my first PR in open source, sorry about that

@Kludex
Copy link
Member

Kludex commented Oct 13, 2024

Can you show me an MRE that this code solves?

@gansik
Copy link
Author

gansik commented Oct 16, 2024

Can you show me an MRE that this code solves?

If you use a different order of routing rules in the example from the documentation, then “Route(‘/’, homepage)” will not be in the generated json schema

routes = [
    Mount('/users', routes=[
        Route('/', users, methods=['GET', 'POST']),
        Route('/{username}', user),
    ]),
    Route('/', homepage)
]

@Kludex
Copy link
Member

Kludex commented Oct 18, 2024

Can you show me an MRE that this code solves?

If you use a different order of routing rules in the example from the documentation, then “Route(‘/’, homepage)” will not be in the generated json schema

routes = [
    Mount('/users', routes=[
        Route('/', users, methods=['GET', 'POST']),
        Route('/{username}', user),
    ]),
    Route('/', homepage)
]

Can you show me the whole code that I can run and see the difference in behavior? I want to add a test...

@Kludex
Copy link
Member

Kludex commented Nov 22, 2024

Ping @gansik 👀

@logan-connolly
Copy link

Although the proposed change is definitely clearer in its intent, I don't think a bug exists with the current implementation due to variable scoping. Here is a simple script to illustrate:

lst = ["A", "B", "C"]

for outer_item in lst:
    print(outer_item)

    # overriding `lst` doesn't affect the outer loop
    lst = [1, 2, 3]

    for inner_item in lst:
        print(inner_item)

I tried to reproduce the bug from #2717 (comment) in a test but couldn't. Everything seems to work fine when mounted routes come before the root mount (I just reused existing handlers from test_schemas.py):

def test_schema_endpoint_pr_example(test_client_factory: TestClientFactory) -> None:
    app = Starlette(
        routes=[
            Mount(
                "/users",
                routes=[
                    Route("/", list_users, methods=["GET"]),
                    Route("/{username}", get_user),
                ],
            ),
            Route("/", OrganisationsEndpoint),
            Route("/schema", endpoint=schema, methods=["GET"], include_in_schema=False),
        ]
    )
    client = test_client_factory(app)
    response = client.get("/schema")

    value = response.text.strip()
    expected = """
    info:
      title: Example API
      version: '1.0'
    openapi: 3.0.0
    paths:
      /:
        get:
          responses:
            200:
              description: A list of organisations.
              examples:
              - name: Foo Corp.
              - name: Acme Ltd.
        post:
          responses:
            200:
              description: An organisation.
              examples:
                name: Foo Corp.
      /users/:
        get:
          responses:
            200:
              description: A list of users.
              examples:
              - username: tom
              - username: lucy
      /users/{username}:
        get:
          responses:
            200:
              description: A user.
              examples:
                username: tom
    """.strip()

    assert value == expected

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.

3 participants