Skip to content

Conversation

@HeverFarber
Copy link
Contributor

@HeverFarber HeverFarber commented Oct 3, 2022

Readme API docs - enforce open API standard that <,> are invalid characters and uniqe operationId.

  • This package usets-json-schema-generator to generate the schema - however ts-json-schema-generator generate <,> when your types include generic types.
  • This package is integrated with @conqa/serverless-openapi-documentation to enrich data. openapi-documentation uses the function name for the operation id.

Solutions:

  • Encode the characters <,> to _
  • Add docs title to each operation id - this is needed when you combine multi openApi files to one (from multi service with the same methods names)

Breaking changes

operationId was change

method.tags = [tagName];
}

method.operationId = `${this.serverless.service.service}-${method.operationId}`;
Copy link
Contributor Author

@HeverFarber HeverFarber Oct 3, 2022

Choose a reason for hiding this comment

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

This field is also responsible for the url also - for example, https://docs.env0.com/reference/project-updatefunc

@HeverFarber HeverFarber marked this pull request as ready for review October 3, 2022 15:06
@HeverFarber HeverFarber requested a review from a team October 3, 2022 15:06
@HeverFarber HeverFarber self-assigned this Oct 3, 2022
Comment on lines 192 to 194
encodeOpenApiToStandard(openApi) {
const INVALID_CHARACTERS_KEY = /<|>/g;
const INVALID_CHARACTERS_REF = /%3C|%3E/g;
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. I would add a comment about what we're doing here and why (how generics are generated with > and how these aren't legal character's in openapi 3.1 spec (with link)
  2. %3C and %3E don't mean much - I would atob('>') to better capture the intent here, and also rename to INVALID_CHARACTERS_ENCODED

Copy link
Contributor Author

@HeverFarber HeverFarber Oct 6, 2022

Choose a reason for hiding this comment

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

btoa - is to encode it to base 64.
Maybe you mean encodeURI

I can change it to something like that - But I prefer to keep it simple. I can add comment about it

new RegExp(`${encodeURI('<')}|${encodeURI('>')/g`)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

);

return mapValuesDeep(mapObject, (value, key) =>
isString(value) && key === '$ref' && INVALID_CHARACTERS_REF.test(value) ?
Copy link
Contributor

Choose a reason for hiding this comment

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

I think isString(value) is superfluous.
If key === '$ref', what else could it be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For safety - but we don't need it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines 196 to 201
const mapObject = mapKeysDeep(openApi, (value, key) =>
INVALID_CHARACTERS_KEY.test(key) ? key.replace(INVALID_CHARACTERS_KEY, '_') : key
);

return mapValuesDeep(mapObject, (value, key) =>
isString(value) && key === '$ref' && INVALID_CHARACTERS_REF.test(value) ?
Copy link
Contributor

Choose a reason for hiding this comment

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

Matter of personal of style so your call - I think the interim value of mapObject does nothing but make this method a little more verbose than it needs to be.
Of course it makes sense to avoid nested functions like f(g(x)).

Deepdash, like lodash does offer a way to chain functions which make it easier to follow the functional path:

_(openApi)
  .mapKeysDeep(stubGenerics)
  .mapValuesDeep(stubBase64EncodedGenerics)
  .value();

Copy link
Contributor Author

@HeverFarber HeverFarber Oct 6, 2022

Choose a reason for hiding this comment

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

I didn't see they offer this chain - they offer to put the functions on the lodash object _ but I think not in the chain (more complex to do it) But I will try. I searched after that in the docs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like how they mix it with lodash and it will not work with the chain also we have typescript problem.

// or cherry-pick method you only need and mix it into lodash
require('deepdash/addFilterDeep')(_);
// or cherry-pick method separately if you don't want to mutate Lodash instance
const filterDeep = require('deepdash/getFilterDeep')(_);

I tried to do something with flow - but it just complex it and increase the code

{
"name": "serverless-openapi-typescript",
"version": "1.0.10",
"version": "2.0.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed major version because we broke the API.
operationId value will be different after this version

@HeverFarber HeverFarber requested review from a team and roni-frantchi October 11, 2022 08:14
@HeverFarber HeverFarber merged commit c7ad2d1 into main Oct 11, 2022
@HeverFarber HeverFarber deleted the chore-add-openapi-encoded branch October 11, 2022 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants