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

fix: validating cookie parameter for req-validator #225

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

Conversation

Prashansa-K
Copy link
Contributor

At the moment, we do not support cookie parameters in request-validator plugin. However, deck file openapi2kong command generates the yaml file even if it is present in the parameter schema. Thus, adding a validation layer over it to ensure deck does not create invalid specs.

For: #223

At the moment, we do not support cookie parameters in request-validator
plugin. However, `deck file openapi2kong` command generates the yaml file
even if it is present in the parameter schema. Thus, adding a validation
layer over it to ensure deck does not create invalid specs.

For: #223
@Prashansa-K Prashansa-K self-assigned this Nov 11, 2024
@Prashansa-K Prashansa-K force-pushed the fix/validate-for-cookie-presence branch from 2fef98b to 854f110 Compare November 11, 2024 06:27
@codecov-commenter
Copy link

codecov-commenter commented Nov 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.06%. Comparing base (a214fdd) to head (b67debf).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #225      +/-   ##
==========================================
+ Coverage   68.97%   69.06%   +0.08%     
==========================================
  Files          24       24              
  Lines        3343     3352       +9     
==========================================
+ Hits         2306     2315       +9     
  Misses        864      864              
  Partials      173      173              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines 86 to 87
return nil, fmt.Errorf(`cookie parameters are not supported for request-validator plugin;
choose either path, query or header`)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it should throw an error, but should log a warning, and continue without generating the validation logic.

Similar to bodies, where the content type isn't json.

Copy link
Member

Choose a reason for hiding this comment

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

and most likely we should make it more explicit in the docs, that Cookie isn't supported

At the moment, we do not support cookie parameters in request-validator
plugin. However, `deck file openapi2kong` command generates the yaml file
even if it is present in the parameter schema. Thus, adding a validation
layer over it to ensure deck does not create invalid specs.

For: #223
@Prashansa-K Prashansa-K force-pushed the fix/validate-for-cookie-presence branch from cf85ec6 to b67debf Compare November 13, 2024 07:30
Comment on lines +73 to +74
logbasics.Info(`cookie parameters are not supported for request-validator plugin;
choose either path, query or header`)
Copy link
Member

Choose a reason for hiding this comment

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

these backticks will preserve the whitespace and newline for the log message, which is not what we want. Also the message can be a bit clearer about the consequences I think, how about "cookie parameters are not supported by the request-validator plugin; validation will be skipped"

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