fix: URL routing to accept requests that contain unused parameters

[mentallydetached]

Fix: URL routing to consider all parameters without failing to route requests that contain additional unused artifacts.

Untested and still refining. Unsure if the logic should exist somewhere else in the project.

[mryel00]

Please use more descriptive commit messages and try to follow the conventional commits guidelines.

[mentallydetached]

Please use more descriptive commit messages and try to follow the conventional commits guidelines.

Did a quick glance through the conventional commits approach. Tried to follow it for the commit descriptions. Not sure if that convention should apply to commit headers as well. Please advise

[mryel00]

Sry I actually miswrote that 😅. I meant the commit header not message there but please use it for both 😄

[mentallydetached]

FYI: Tried writing unit tests for the URL parsing, but couldn't wrap my head around the function -> class -> function situation with run_server. Will gladly provide unit tests if someone is able to create an example of how we can mock a request to do_GET. With that being said, I did manually test the URL parsing logic and all seems to be functional on my side.

[roamingthings]

@mentallydetached Thank you for your contribution.

Regarding the tests you mentioned we have to look into it. The original codebase also misses theses kinds of tests.

[roamingthings]

@mentallydetached Can you provide some example URLs that contain these unwanted artefacts?

[mentallydetached]

@mentallydetached Can you provide some example URLs that contain these unwanted artefacts?

Yeah, sorry I worded that really awkwardly. Specifically what I'm addressing here is that the old logic (simple == comparison) doesn't route requests properly in certain scenarios. The artifacts I'm referring to are things like anchor tags, unused parameters, extra slashes, etc.

This is an issue that causes incompatibility with Fluidd because it is passing a fake parameter (cacheBuster) to prevent a browser from serving the same image repeatedly, but Spyglass doesn't ignore it.

Here's a table showing what I'm solving. This example assumes you are attempting to stream and you've configured the stream_url as "/stream?action=stream"

Example URL	Expected function	Actual behavior
/stream?action=stream	start_streaming	start_streaming served
/stream/?action=stream	start_streaming	404 served
/stream?action=stream&cacheBuster=val	start_streaming	404 served
/stream?test=val&action=stream	start_streaming	404 served
/stream?action=stream#	start_streaming	404 served

[mentallydetached]

To clarify, those aren't the only scenarios this addresses. If someone sets their stream_url to contain 2 or more parameters. They are going to also be locked in to that exact order. My code solves for that as well

[roamingthings]

Thanks for sharing the examples. I have to admit that I'm not (yet) familiar with the Python ecosystem. I expected that the library that is used already does some kind of cleanup/splitting the given URL.

[mentallydetached]

Thanks for sharing the examples. I have to admit that I'm not (yet) familiar with the Python ecosystem. I expected that the library that is used already does some kind of cleanup/splitting the given URL.

This is true depending on the module you use for your server. I don't think that's possible with the http server you are using without doing your own URL parsing. It's possible there is some built in method within http that does this that I'm not aware of.

Alternatively, you could import a more comprehensive web framework.

[roamingthings]

We want to keep the number of additional frameworks as low as possible. So I would say it's ok to have some manual parsing. I will take a deeper look at the pr this weekend.

[mentallydetached]

We want to keep the number of additional frameworks as low as possible. So I would say it's ok to have some manual parsing. I will take a deeper look at the pr this weekend.

Sure, and as I mentioned before I'll be more than happy to build out all of the test cases if you can figure out how to mock the nested do_GET() function.

[roamingthings]

@mentallydetached you've written

Unsure if the logic should exist somewhere else in the project.

I think it would make sense to have these functions in a separate module/file. I love the single responsibility principle and It would allow for easier testing.

Nevertheless I will try to find out how to mock/test routing functions.

[roamingthings]

@mentallydetached I've played around with mocking and haven't been able to properly mock it either.

My suggesting would be to implement and test the methods separately, as we discussed.
I assume that it would be good to change the server implementation to make it more testable. This could also help with USB camera support in #10

[mryel00]

I had some time to test it now and it seems like just /stream isn't working with that parsing anymore.
As I'm not advanced in such a topic, I don't know if it makes sense to support something like /stream and /snapshot that are our current default urls. We should maybe change them anyway. It would be still nice if you could use such short urls but isn't something we really need in my opinion, except for backwards compatibility atm maybe.
Furthermore is it not working like expected by @mentallydetached. /stream/?action=stream fails for /stream?action=stream but your examples suggest that it should work. Did you just make a mistake in your examples maybe or is this a bug?

[mentallydetached]

I had some time to test it now and it seems like just /stream isn't working with that parsing anymore. As I'm not advanced in such a topic, I don't know if it makes sense to support something like /stream and /snapshot that are our current default urls. We should maybe change them anyway. It would be still nice if you could use such short urls but isn't something we really need in my opinion, except for backwards compatibility atm maybe. Furthermore is it not working like expected by @mentallydetached. /stream/?action=stream fails for /stream?action=stream but your examples suggest that it should work. Did you just make a mistake in your examples maybe or is this a bug?

That would be a bug for sure. I've been out of town for the past week but will take a look at it when I get back. I also saw the refactor from the other issue you linked with USB support, I think if we implement that structure we'll be able to easily mock the service and build the functionality with unit tests so we can be confident there are no bugs before merging.

[mryel00]

Don't refactor it like I did in #10 this was more like a proof of concept that I can test that feature.
I think it should be more like @roamingthings described earlier:

I think it would make sense to have these functions in a separate module/file. I love the single responsibility principle and It would allow for easier testing.

This would better compare to #37.

[mryel00]

Hey @mentallydetached,
long time no hear. I finally got some time to work myself into that thing. Did you already did some work on it in the past 2 months? Otherwise I will try to get it all done asap

[mentallydetached]

Hey @mentallydetached, long time no hear. I finally got some time to work myself into that thing. Did you already did some work on it in the past 2 months? Otherwise I will try to get it all done asap

Sorry, haven't touched it since. I just had made the original modification locally and have been using it successfully with fluidd, but never got the time to work on the updates to the PR.

[mryel00]

@roamingthings please have a look at this if I have missed something. Maybe you have a better idea on how to make the tests?

[roamingthings]

Looks good