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

Fixed regular expression for tags #628

Merged
merged 1 commit into from
Mar 13, 2020

Conversation

mkanoor
Copy link
Contributor

@mkanoor mkanoor commented Mar 11, 2020

This is to fix an issue in the openapi generator
When we have regular expressions they come in 2 formats

  1. /pattern/
  2. "pattern"

In the first one if the pattern contains a forward slash
(aka Solidus from
https://www.ecma-international.org/archive/ecmascript/2013/GA/ga-2013-076.pdf)
it needs to be escaped with a backslash (reverse solidus)

In the second case we dont have to escape the forward slash

In our openapi spec we have always tended to use the second format.

The openapi generator checks the first charachter and if it starts
with a forward slash it assumes it's the first format and doesn't
escape the / contained in the string

https://github.com/OpenAPITools/openapi-generator/blob/de2753dfc7d17a8afcc14fdd705ade3f6cca3cb2/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java#L4825

In our tags we are expecting values like
/approval/workspace=55

Since our regexp starts with a / it bypasses the escaping process in the
openapi generator

To temporarily resolve this issue I have modified our regular expression to have a ^
which bypasses the first charachter to be / in the openapi-generator

I have opened an issue with the openapi-generator repository which might
resolve the issue on the generator
OpenAPITools/openapi-generator#5582

This is to fix an issue in the openapi generator
When we have regular expressions they come in 2 formats
1. /pattern/
2. "pattern"

In the first one if the pattern contains a forward slash
(aka Solidus from
https://www.ecma-international.org/archive/ecmascript/2013/GA/ga-2013-076.pdf)
it needs to be escaped with a backslash (reverse solidus)

In the second case we dont have to escape the forward slash

In our openapi spec we have always tended to use the second format.

The openapi generator checks the first charachter and if it starts
with a forward slash it assumes it's the first format and doesn't
escape the / contained in the string

https://github.com/OpenAPITools/openapi-generator/blob/de2753dfc7d17a8afcc14fdd705ade3f6cca3cb2/modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java#L4825

In our tags we are expecting values like
/approval/workspace=55

Since our regexp starts with a / it bypasses the escaping process in the
openapi generator

To temporarily resolve this issue I have modified our regular expression to have a ^
which bypasses the first charachter to be / in the openapi-generator

I have opened an issue with the openapi-generator repository which might
resolve the issue on the generator
OpenAPITools/openapi-generator#5582
Copy link

@lindgrenj6 lindgrenj6 left a comment

Choose a reason for hiding this comment

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

LGTM! I didn't even think about using /pattern/ compared with "pattern", hopefully they can get it fixed on the openapi side since this is a pretty big issue since it only supports the second format.

Validated with @syncrou's catalog-api-client-test repo that it generates properly for both ruby and python.

Copy link
Contributor

@syncrou syncrou left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@syncrou syncrou merged commit 889b201 into RedHatInsights:master Mar 13, 2020
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.

None yet

4 participants