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

Introduce faster and richer way to add objects as query parameters #1974

Closed
wants to merge 12 commits into from
Closed

Introduce faster and richer way to add objects as query parameters #1974

wants to merge 12 commits into from

Conversation

abnercpp
Copy link

@abnercpp abnercpp commented Nov 18, 2022

Description

This PR is a result of the discussion in PR#1897. It introduces a new way of adding objects as query parameters to a RestRequest instance when the object type is known beforehand, which allows for a lot of optimizations to be made.

Apart from this, this PR also builds on top of pre-existing functionality, allowing for things like:

  • CSV and Array format is now available for all IEnumerable types.
  • Formatting is applied to all formattable elements in an IEnumerable property.
  • Properties are cached and getters are compiled into a delegate using LINQ Expressions for better performance.
  • If a property is not IConvertible, IEnumerable or IFormattable, use the type's defined TypeConverter and delegate conversion to string to it instead of using .ToString(). This allows users to create custom converters to determine how certain types should be stringified. If no custom converter is defined, we will fall back to .ToString() anyways.
  • Ref structs are properly ignored since they cannot be boxed and don't play well with reflection, which is not taken into account in the AddObject implementation.

Purpose

This pull request is a:

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@dnfadmin
Copy link

dnfadmin commented Nov 18, 2022

CLA assistant check
All CLA requirements met.

@alexeyzimarev
Copy link
Member

Thanks for the PR. Please remember to sign the CLA.

@abnercpp
Copy link
Author

Thanks for the PR. Please remember to sign the CLA.

Oops, my bad. Done. ✅

@abnercpp
Copy link
Author

abnercpp commented Nov 19, 2022

I'll look into what's happening with the test case that failed once I get home.

@abnercpp
Copy link
Author

abnercpp commented Nov 21, 2022

The test case that was failing was one that I had in place to ensure that ref structs are not taken into consideration when turning an object into query parameters, since they cannot be boxed and will throw at runtime, which is what happens with the default AddObject extension method.

I added a comment explaining the fix, but basically, I was filtering out properties based on whether they had a IsByRefLikeAttribute defined, but this attribute is generated by the compiler, and different assemblies will have different versions of this attribute, which was making this check useless. So now we're just checking if the property return type has any attributes with the full name "System.Runtime.CompilerServices.IsByRefLikeAttribute" instead.

@stale
Copy link

stale bot commented Jan 7, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jan 7, 2023
@repo-ranger
Copy link

repo-ranger bot commented Jan 7, 2023

⚠️ This issue has been marked wontfix and will be closed in 3 days

@alexeyzimarev
Copy link
Member

I had to merge it manually, for some reason GitHub doesn't recognise it.

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

3 participants