Skip to content

First pass at basic query string parameters. #46545

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

Merged

Conversation

mitchdenny
Copy link
Member

@mitchdenny mitchdenny commented Feb 9, 2023

First pass at basic string binding for request delegate source generator

This PR gets the basics of binding strings to the source generated request delegate. It builds on the special type parameter handling. The PR includes logic to detect the IFromX metadata interfaces and set the parameter type accordingly (this is the explicit handling logic discussed.

It includes partial null handling in that it will return a 400 if StringValues is null or empty - but it doesn't do full on nullability handling around things like nullable arrays, arrays of nullables, and nullable arrays of nullables.

You can see where I've got a pattern matching switch to detect what kind of parameter preparation logic to emit. The idea here is that if we have a particular special case which is messy to handle with just one block of code we can use this switch to provide alternative implementations, but mostly it'll just switch on the Source property.

(note: some tests aren't passing yet because I haven't updated the baselines - that'll be the last thing I do)

@mitchdenny
Copy link
Member Author

@captainsafia here is the minimal PR that I think will help us avoid stepping on each others toes.

@mitchdenny mitchdenny requested review from a team, dougbu and wtgodbe as code owners February 13, 2023 03:14
Copy link
Member

@wtgodbe wtgodbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please undo the submodule changes

Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM for the generator changes

It looks like there have been some unintended changes made to the git submodules. I'd revert those before merging.

Copy link
Member

@captainsafia captainsafia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving since I'm fine with the generator changes and @wtgodbe has already requested changes for the submodule changes I was nervous about. 😅

@mitchdenny
Copy link
Member Author

Please undo the submodule changes

Done. Sorry about that. GitHub was giving me issues yesterday not recognizing a push that I had made, and I did a sloppy whitespace change and ended up pulling in the submodules update inadvertently.

@dougbu
Copy link
Contributor

dougbu commented Feb 13, 2023

Please undo the submodule changes

Done. Sorry about that. GitHub was giving me issues yesterday not recognizing a push that I had made, and I did a sloppy whitespace change and ended up pulling in the submodules update inadvertently.

git config --global --add submodule.recurse true works wonders and will affect all future aspnetcore clones you make.

@mitchdenny mitchdenny requested a review from wtgodbe February 14, 2023 00:42
@mitchdenny mitchdenny enabled auto-merge (squash) February 14, 2023 01:42
@mitchdenny mitchdenny merged commit 1a70e36 into dotnet:main Feb 14, 2023
@ghost ghost added this to the 8.0-preview2 milestone Feb 14, 2023
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.

5 participants