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

Add support for multiple @ResponseSchema decorators using oneOf. #58

Merged
merged 6 commits into from
Sep 3, 2020

Conversation

godwinpang
Copy link
Contributor

Missing some docs, but functionality + tests are there. TS isn't a language I'm very familiar with and this is my first time using lodash, so I'd appreciate any comments/suggestions!

Closes #53

@codecov
Copy link

codecov bot commented Sep 2, 2020

Codecov Report

Merging #58 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #58      +/-   ##
==========================================
+ Coverage   99.49%   99.52%   +0.02%     
==========================================
  Files           4        4              
  Lines         199      209      +10     
  Branches       61       66       +5     
==========================================
+ Hits          198      208      +10     
  Misses          1        1              
Impacted Files Coverage Δ
src/decorators.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c32c925...1f8ddcd. Read the comment docs.

Also wrote two tests for that. All tests passed when I ran it.
Comment on lines 129 to 153
const oldSchema =
source.responses[statusCode]?.content[contentType].schema
if (oldSchema?.$ref || oldSchema?.items?.$ref) {
// case where we're adding multiple schemas under single statuscode/contentType
// with single $ref
const isOldSchemaArray = oldSchema?.items?.$ref

// delete old schema and integrate into current schema under oneOf
const schemaObj = { oneOf: [{ ...oldSchema }, schema] }
responses[statusCode].content[contentType].schema = schemaObj

if (isOldSchemaArray) {
delete oldSchema.items
delete oldSchema.type
} else {
delete oldSchema.$ref
}
} else if (oldSchema?.oneOf) {
// case where there's already multiple existing schemas
const oneOf = _.concat([...oldSchema.oneOf], schema)
responses[statusCode].content[contentType].schema = { oneOf }
delete oldSchema.oneOf
}

return _.merge({}, source, { responses })
Copy link
Owner

Choose a reason for hiding this comment

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

I'm thinking all the deletes makes this a bit hard to read. How about splitting into two branches, something like

if (oldSchema) {
  const newSchema = {};

  // ...construct newSchema

  source.responses[statusCode].content[contentType].schema = newSchema
  return source
} else {
  return _.merge({}, source, { responses }) // just as before
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed here

src/decorators.ts Outdated Show resolved Hide resolved
src/decorators.ts Outdated Show resolved Hide resolved
Comment on lines 144 to 152
const newStatusCodeResponse = _.merge(
{},
source.responses[statusCode],
responses[statusCode]
)
const newSchema = makeNewSchema()
newStatusCodeResponse.content[contentType].schema = newSchema
source.responses[statusCode] = newStatusCodeResponse
return source
Copy link
Owner

Choose a reason for hiding this comment

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

Is there any way we could avoid the _.merge here? How about just assigning to source.responses[statusCode].content[contentType].schema = ...?

Also, I'm not sure if extracting makeNewSchema into another function provides much additional value. We can probably replace it with a simple ternary: const newSchema = oldSchema.oneof ? ... : ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I'm not sure if extracting makeNewSchema into another function provides much additional value. We can probably replace it with a simple ternary: const newSchema = oldSchema.oneof ? ... : ...

I agree

Is there any way we could avoid the _.merge here? How about just assigning to source.responses[statusCode].content[contentType].schema = ...?

For this, my original thought with leaving the _.merge in was to keep descriptions for statusCodes. Consider the following example

@ResponseSchema(Model1)
@ResponseSchema(Model2, {description: xxx})

If we drop merge and just set schema, then I think that the first description would be lost. Lmk what you think.

Copy link
Owner

Choose a reason for hiding this comment

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

Right, better leave the merge in there for now then.

Copy link
Owner

@epiphone epiphone left a comment

Choose a reason for hiding this comment

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

Great, thanks! Did you want to add docs before we get this merged?

@godwinpang
Copy link
Contributor Author

yeah - I'll update some docs. thanks for the fast reviews!

@godwinpang
Copy link
Contributor Author

Just wondering, how long would it take for these changes to make it onto the npm registry after merging? Our team would love to use this soon :)

@epiphone epiphone merged commit 12fad34 into epiphone:master Sep 3, 2020
@epiphone
Copy link
Owner

epiphone commented Sep 3, 2020

It's out now in v2.1.0

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.

Multiple ResponseSchemas
2 participants