-
Notifications
You must be signed in to change notification settings - Fork 241
Fix model bindng #3479
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
Fix model bindng #3479
Conversation
Other cleanup and improvements
| RequireAuthorization(). | ||
| WithOpenApi(). | ||
| ProducesProblem(401); | ||
| Accepts<DownstreamApiOptions>(true, MediaTypeNames.Application.Json). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was needed for the model binding
| options.WithAgentIdentity(requestParameters.AgentIdentity); | ||
| } | ||
|
|
||
| if (!string.IsNullOrEmpty(tenant)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed this, it duplicates what is in DownstreamAPI.AcquireTokenOptions
| app.MapGet("/Validate", ValidateEndpoint). | ||
| WithName("Validate Authorization header"). | ||
| RequireAuthorization(). | ||
| WithOpenApi(). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not needed when using net9 built in open api support
| return TypedResults.Ok(new AuthorizationHeaderResult(result)); | ||
| } | ||
|
|
||
| public static DownstreamApiOptions MergeDownstreamApiOptionsOverrides(DownstreamApiOptions left, DownstreamApiOptions right) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this shouldn't have been in this class, moved
| [FromQuery] string? agentUsername, | ||
| [FromQuery] string? tenant, | ||
| [FromBody] DownstreamApiOptions? optionsOverride, | ||
| [AsParameters] AuthorizationHeaderRequest requestParameters, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The inputs can be collected into one struct when marked with AsParameters
jmprieur
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @keegan-caruso
Other cleanup and improvements Co-authored-by: Keegan Caruso <[email protected]>
Other cleanup and improvements Co-authored-by: Keegan Caruso <[email protected]>
Other cleanup and improvements Co-authored-by: Keegan Caruso <[email protected]>
Other cleanup and improvements Co-authored-by: Keegan Caruso <[email protected]>
* initial commit * Refactor, add basic error handling * Adding E2E test (#3476) * Add authorization header endpoint, minor cleanup * more end to end tests for sidecar (#3477) * Adding E2E test * Improving the tests * Fix name * Apply suggestion from @keegan-caruso Co-authored-by: Keegan <[email protected]> --------- Co-authored-by: Keegan <[email protected]> * Add more tests * Improvements (#3478) * Fix model bindng (#3479) Other cleanup and improvements Co-authored-by: Keegan Caruso <[email protected]> * Update tests to new behavior + 401 on AuthorizationHeader when we are sure it's an auth issue (#3480) * Improvements * fix tests with new behavior * Apply suggestion from @keegan-caruso Co-authored-by: Keegan <[email protected]> * Apply Keegan's suggestion --------- Co-authored-by: Keegan <[email protected]> * add downstream api (#3483) * add downstream api --------- Co-authored-by: Keegan Caruso <[email protected]> * Enable container workflow (#3496) * containerization * Enable container workflows --------- Co-authored-by: Jean-Marc Prieur <[email protected]> Co-authored-by: Keegan Caruso <[email protected]> * Add more e2e tests (#3504) Co-authored-by: Keegan Caruso <[email protected]> * Authorization Header and downstream API endpoint updates (#3507) * Authorization Header and downstream API endpoints updates * Fix open api generation --------- Co-authored-by: Keegan Caruso <[email protected]> * Sidecar endpoint descriptions (#3510) * Add more descriptions to open api document * Adjust for open api doc generation --------- Co-authored-by: Keegan Caruso <[email protected]> * Current implementation of Open API description generation is not trim friendly * Add agentuserid to list of params (#3514) * Add agentuser id to list of params * Apply suggestion from @jmprieur Co-authored-by: Jean-Marc Prieur <[email protected]> * Apply suggestion from @jmprieur Co-authored-by: Jean-Marc Prieur <[email protected]> * update openapi file --------- Co-authored-by: Keegan Caruso <[email protected]> Co-authored-by: Jean-Marc Prieur <[email protected]> * Add windows container (#3516) Co-authored-by: Keegan Caruso <[email protected]> * Sidecar python adapter devapp (#3508) * Python adapter * feedback --------- Co-authored-by: Keegan Caruso <[email protected]> * Add readme (#3517) * Add readme * Move runtime composition details in README move runtime composition section --------- Co-authored-by: Keegan Caruso <[email protected]> * Make the sidecar trim friendlier (#3518) Co-authored-by: Keegan Caruso <[email protected]> * Update python adapter (#3519) Co-authored-by: Keegan Caruso <[email protected]> * Don't use R2R (#3523) Co-authored-by: Keegan Caruso <[email protected]> * Exclude EndpointsE2ETests when ran from GH action * include FROM_GITHUB_ACTION in sidecar tests * Use in-memory config for sidecar e2e test * Apply suggestions from code review Co-authored-by: Jean-Marc Prieur <[email protected]> * Move SidecarApiFactory to separate file * Also needs test filter --------- Co-authored-by: Keegan Caruso <[email protected]> Co-authored-by: Jean-Marc Prieur <[email protected]>
No description provided.