litestar-org / litestar

Production-ready, Light, Flexible and Extensible ASGI API framework | Effortlessly Build Performant APIs
https://litestar.dev/
MIT License
5.62k stars 382 forks source link

Duplicate path parameters should raise exception #204

Closed danesolberg closed 2 years ago

danesolberg commented 2 years ago

~Just beginning to learn the codebase, but excited to contribute something substantial in the future 😄

Currently, routes with duplicate path parameters can be registered to a handler. The conflict is resolved by using the value associated to the last occurrence of the duplicated parameters (see here).

I would think an exception should be raised when registering the route, or at least that's the behavior I'm used to seeing. The test below currently passes, as a demonstration.

Current behavior

def test_that_currently_passes() -> None:
    @get(path=["/{some_id:int}/foo/{some_id:int}"], media_type=MediaType.TEXT)
    def handler_fn(some_id: int) -> str:
        return str(some_id)

    with create_test_client(handler_fn) as client:
        resp = client.get("/1/foo/2")
        assert resp.status_code == HTTP_200_OK
        assert resp.text == "2"

Proposed behavior

def test_duplicate_path_param_validation() -> None:
    @get(path="/{param:int}/foo/{param:int}")
    def handler_fn(param: int) -> None:
        pass

    with pytest.raises(ImproperlyConfiguredException):
        Starlite(route_handlers=[handler_fn])

Possible fix

A simple change when creating the kwargs model that checks for repeated params. Currently param names are added to a set without checking for repeats.

def create_handler_kwargs_model(self, route_handler: BaseRouteHandler) -> KwargsModel:
    """
    Method to create a KwargsModel for a given route handler
    """
    dependencies = route_handler.resolve_dependencies()
    signature_model = get_signature_model(route_handler)
    path_parameters = set()
    for param in self.path_parameters:
        param_name = param["name"]
        if param_name in path_parameters:
            raise ImproperlyConfiguredException(
                f"Duplicate path parameters should not be used. Duplicate parameter {{{param_name}}} found in path {self.path}"
            )
        path_parameters.add(param_name)
    return KwargsModel.create_for_signature_model(
        signature_model=signature_model, dependencies=dependencies, path_parameters=path_parameters
    )


Happy to push this, I just still have a very shallow grasp of the codebase so am probably not doing this the best way!


[!NOTE]
While we are open for sponsoring on GitHub Sponsors and OpenCollective, we also utilize Polar.sh to engage in pledge-based sponsorship.

Check out all issues funded or available for funding on our Polar.sh dashboard

  • If you would like to see an issue prioritized, make a pledge towards it!
  • We receive the pledge once the issue is completed & verified
  • This, along with engagement in the community, helps us know which features are a priority to our users.

Fund with Polar

peterschutt commented 2 years ago

Sounds reasonable to me - push away - a test or two would be appreciated also.