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(converter): add securitySchemeTypeRefractorPlugin #3802

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

kowalczyk-krzysztof
Copy link
Contributor

refs: SWG-9963

@char0n char0n changed the title feat(apidom-converter): add securitySchemeTypeRefractorPlugin feat(converter): add securitySchemeTypeRefractorPlugin Feb 6, 2024
@kowalczyk-krzysztof kowalczyk-krzysztof force-pushed the feat/mutualtls-refractor branch 2 times, most recently from 555bcdd to d39d076 Compare February 6, 2024 15:48
@kowalczyk-krzysztof kowalczyk-krzysztof marked this pull request as draft February 7, 2024 14:21
@kowalczyk-krzysztof
Copy link
Contributor Author

As we discussed, the strategy is going to be different so I'm converting this PR to draft as I'm applying those changes.

@kowalczyk-krzysztof
Copy link
Contributor Author

@char0n I updated the solution but I'm not sure if I'm handling it correctly (checking for name match). Could use some more feedback.

@char0n
Copy link
Member

char0n commented Feb 12, 2024

We now have working OpenApi3_1Element visitor hook. I'm now looking at ComponentsElement hook.

@kowalczyk-krzysztof
Copy link
Contributor Author

@char0n Two things I'm unsure about:

  1. When exactly should I push annotations? Is the current implementation too much?
  2. As it is, the mutated SecurityRequirements looks like the code below - there is an empty object left. Should something be done about it?
"security": [
          {},
          {
            "apiKey-scheme": [
              "read",
              "write"
            ]
          }
        ],

@kowalczyk-krzysztof kowalczyk-krzysztof marked this pull request as ready for review February 12, 2024 09:54
@char0n
Copy link
Member

char0n commented Feb 12, 2024

  1. When exactly should I push annotations? Is the current implementation too much?

Good question. Let me think about this for a minute.

  1. As it is, the mutated SecurityRequirements looks like the code below - there is an empty object left. Should something be done about it?
"security": [
          {},
          {
            "apiKey-scheme": [
              "read",
              "write"
            ]
          }
        ],

We don't care. Empty Security Requirement Object is valid as empty object according to OpenAPI 3.0.x spec.

@char0n
Copy link
Member

char0n commented Feb 12, 2024

When exactly should I push annotations? Is the current implementation too much?

I guess this is dependent on the broader strategy how annotations should be generated. Do we want to generate annotation for every removed Schema Element or just generate one for the entire plugin?

I currently see two possible strategies for this. Let's stay in the realm of Security Scheme Object and model the strategies to that usecase.

Generate single annotation for plugin run

We'll just introduce semaphore variable into the plugin and in post we'll add the annotation if semaphore is set to true. semaphore is set to true whenever the Security Scheme Object is removed from the definition.

Another option is to add annotation when first Security Scheme Object is removed and guard that it's not added later again. This has advantage over the above post solution in retaining the real order of annotations as they are added right when they are generated.

Generate annotations for every element

Now this one would be more complex. We add annotation for every removal. Along with that we will translate element source maps to the annotation. This means that this single plugin can generate number of annotations and each annotation will contain specific source map information about the removed Security Scheme Object.

@kowalczyk-krzysztof
Copy link
Contributor Author

When exactly should I push annotations? Is the current implementation too much?

I guess this is dependent on the broader strategy how annotations should be generated. Do we want to generate annotation for every removed Schema Element or just generate one for the entire plugin?

I think users of the converter would prefer to see information about all changes to the API definition and where they happened. So IMO Generate annotations for every element would work best.

@char0n
Copy link
Member

char0n commented Feb 12, 2024

I think users of the converter would prefer to see information about all changes to the API definition and where they happened. So IMO Generate annotations for every element would work best.

All right, decided then.

Copy link
Member

@char0n char0n left a comment

Choose a reason for hiding this comment

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

LGTM

@kowalczyk-krzysztof kowalczyk-krzysztof merged commit 2666194 into main Feb 12, 2024
8 checks passed
@kowalczyk-krzysztof kowalczyk-krzysztof deleted the feat/mutualtls-refractor branch February 12, 2024 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants