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/multiple securities #141

Merged
merged 3 commits into from
Jan 15, 2025
Merged

Feat/multiple securities #141

merged 3 commits into from
Jan 15, 2025

Conversation

TheEdward162
Copy link
Contributor

@TheEdward162 TheEdward162 commented Jan 7, 2025

  • TODO: this is based on Revive #140 so those changes will show up too until it is merged.

Description

Adds support for specifying multiple securities in a map.

Motivation and Context

There's been a request to be able to specify in map that I want to use first valid security from a given list. There's also been a request before to allow applying multiple securities onto one request. This PR introduces a backwards-compatible way to do both.

Types of changes

  • Bug fix (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 change)

@TheEdward162 TheEdward162 requested a review from freaz January 14, 2025 14:44
@@ -11,7 +11,8 @@ export type FetchOptions = {
headers?: MultiMap,
query?: MultiMap,
body?: AnyValue,
security?: string,
/** Security configs to apply to this request. Specifying a string is equal to using `first-valid` */
security?: string | { kind: 'first-valid', ids: string[] } | { kind: 'all', ids: string[] },
Copy link
Member

Choose a reason for hiding this comment

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

@Jakub-Vacek @janhalama what do you think about this API? Most importantly won't it be trouble for Edgar?

Copy link
Member

@Jakub-Vacek Jakub-Vacek Jan 15, 2025

Choose a reason for hiding this comment

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

I think this will be fine for newer version(s) of Edgar where we use stronger models. We will have to explain LLM difference between first-valid and all and nudge him towards first valid
:)

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good to me.

This is my workaround for using multiple security schemes with Pipedrive. It will be much easier to implement it now including the fallback to next security scheme in case of 401 error.

Copy link
Member

Choose a reason for hiding this comment

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

As it is implemented now, it will just resolve first security. Not do call and try another.
For Pipedrive it was migration from api_key to oauth bearerToken (btw that naming "consistency" 🙈). But in general how often users have both configured?

Copy link
Member

@freaz freaz left a comment

Choose a reason for hiding this comment

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

Overall looks good, thanks. I'd just wait for feedback from Jakub and Jan, because they recently did most of the comlinks.

* Implement support for multiple securities
* The map can pick between `first-valid` and `all` kinds, which affect whether all specified securities are applied or just the first valid
* move "legacy" http security normalization to map-std, so that core input surface doesn't increase
@TheEdward162 TheEdward162 force-pushed the feat/multiple-securities branch from 99cbca0 to 32c0743 Compare January 15, 2025 21:40
@TheEdward162 TheEdward162 merged commit 141ef1f into main Jan 15, 2025
6 checks passed
@TheEdward162 TheEdward162 deleted the feat/multiple-securities branch January 15, 2025 21:53
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.

4 participants