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: fix existing query values bug #1737

Merged
merged 3 commits into from
Jun 26, 2024

Conversation

TimothyMakkison
Copy link
Contributor

@TimothyMakkison TimothyMakkison commented Jun 25, 2024

Closes #1732
Fixes a bug where existing query parameters would have their positions reversed #1732

  • Extracted logic into a method
  • If the existing Query is empty we early return
    • This early return prevents the allocation of an empty 300 byte NameValueCollection
  • Otherwise the query is parsed with each key pair added
    • There might be room for improvement here by using a linq Select method. Select would know the length of the collection and InsertRange would be able to bulk copy the values. This could only be done if we know for certain that the key can't be empty and Where is not required.

Before

Method Mean Error StdDev Gen0 Gen1 Allocated
ConstantRouteAsync 2.606 us 0.0251 us 0.0235 us 0.7668 0.0038 7.06 KB
DynamicRouteAsync 3.306 us 0.0589 us 0.0550 us 0.8087 - 7.45 KB
ComplexDynamicRouteAsync 4.635 us 0.0730 us 0.0683 us 0.8926 0.0076 8.27 KB
ObjectRequestAsync 5.169 us 0.0992 us 0.0974 us 0.9766 0.0076 8.99 KB
ComplexRequestAsync 13.306 us 0.1515 us 0.1343 us 1.6479 - 15.24 KB

Query changes

Method Mean Error StdDev Gen0 Gen1 Allocated
ConstantRouteAsync 2.696 us 0.0302 us 0.0236 us 0.7362 0.0038 6.77 KB
DynamicRouteAsync 3.369 us 0.0666 us 0.0684 us 0.7782 - 7.16 KB
ComplexDynamicRouteAsync 4.529 us 0.0412 us 0.0344 us 0.8621 - 7.97 KB
ObjectRequestAsync 5.061 us 0.0476 us 0.0445 us 0.9384 0.0076 8.69 KB
ComplexRequestAsync 13.274 us 0.1464 us 0.1222 us 1.6174 0.0153 14.94 KB

Copy link

codecov bot commented Jun 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.63%. Comparing base (6ebeda5) to head (7291cca).
Report is 47 commits behind head on main.

Current head 7291cca differs from pull request most recent head 713e067

Please upload reports for the commit 713e067 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1737      +/-   ##
==========================================
- Coverage   87.73%   84.63%   -3.11%     
==========================================
  Files          33       36       +3     
  Lines        2348     2512     +164     
  Branches      294      327      +33     
==========================================
+ Hits         2060     2126      +66     
- Misses        208      304      +96     
- Partials       80       82       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ChrisPulman
Copy link
Member

Nice, I really want to make some kind of live test, for example two console apps that can execute to test the functionality more fully. There's many parts of the code which are either untestable or have no existing tests.
If you have any ideas how we can achieve this, please shout.

@TimothyMakkison
Copy link
Contributor Author

I really want to make some kind of live test, for example two console apps that can execute to test the functionality more fully.

Like a sandbox project? It could be a single file with an interface and dummy http request handler.

There's many parts of the code which are either untestable or have no existing tests.

Which parts did you have in mind? When experimenting with the source generator I have added snapshot testing. The current system is okay because the generator is basic, but any future improvements will need a better test setup. See mapperly as a good example (not biased)😄

@ChrisPulman ChrisPulman enabled auto-merge (squash) June 26, 2024 19:55
@ChrisPulman ChrisPulman merged commit 151b1d9 into reactiveui:main Jun 26, 2024
1 check passed
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.

[Bug]: Query parameters in path are reversed
2 participants