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

Allow deep merging of conflicting definitions #74

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

dbryar
Copy link

@dbryar dbryar commented Jul 22, 2022

  • Changes the behaviour of the security schema to merge, instead of ignore multiple
  • Adds a deepMerge flag to the dispute object to allow merging at a component level

@dbryar
Copy link
Author

dbryar commented Jul 22, 2022

I've also updated the typescript linting and parsing to a more modern version, to remove dependency conflicts, and updated the can-i-use version

@robertmassaioli
Copy link
Owner

Thanks for this pr! I'll review it properly soon. Cursory read idle thoughts:

  • mergeDeep: Might be better to re-engage it for now to be specific.
  • I'm not yet grocking how this would be desirable behaviour for all merging behaviour. Need to think it through further, maybe reread it more or have you explain it further.

@dbryar
Copy link
Author

dbryar commented Jul 22, 2022

Appreciate the quick response.

Under issues there is a call to merge instead of ignore for securityScheme, with a few thumbs. The first change was to simply alter that component to merge (instead of ignore) multiples.

The deep merge option, a new option under the dispute handler, is optional across all components, but written specifically for securityScheme. This is because in a microservice architecture where the services define their security, the IDP may issue different scopes for different services in the JWT of an OAuth based system. Thus, the provider security scheme needs to have all the scopes (as well as other elements) merged, under the same identity. They cannot be indexed with a suffix, nor separated with a prefix.

@dbryar
Copy link
Author

dbryar commented Jul 22, 2022

Also, some of the dependencies are out of date. I have updated some dev dependencies to fix some confilicts

@dbryar
Copy link
Author

dbryar commented Sep 1, 2022

@robertmassaioli bumping PR before I fork

@kmcweeney
Copy link

Just made the same change to the securityscheme section, would love to see this merged into the project so I don't have to fork.

@robertmassaioli
Copy link
Owner

I've been a bit delayed lately because I'm on Paternity leave. I am going to be a bit slow to respond and, if you want to fork then you are more than welcome to.

In the meantime, it seems that the entire purpose of this feature is to fix the merging behaviour for securitySchemes and you make a solid argument for that. The merging behaviour is currently broken.

Because we are really trying to solve something specific maybe we should just make this feature apply to securitySchemes only and we can rename the option to mergeSecuritySchemes and potentially even make it default to true or something like that. Is that possible and would that meet your need? Do you have any other need for the other merging behaviour?

@dbryar
Copy link
Author

dbryar commented Sep 3, 2022

Do you have any other need for the other merging behaviour?

I also need to allow duplicate operationId as we use them to define which Lambda function to call, so that was my next change; to allow a flag that ignores the patch from Issue #7

@ghost
Copy link

ghost commented Aug 9, 2023

is that project dead now ?

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.

3 participants