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 using @Hidden() decorator on controllers #498

Merged
merged 3 commits into from
Oct 7, 2019

Conversation

ryankeener
Copy link
Contributor

@ryankeener ryankeener commented Oct 3, 2019

Adds support for the @Hidden() decorator to controllers which will hide all of its endpoints.

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you written unit tests?
  • Have you written unit tests that cover the negative cases (i.e.: if bad data is submitted, does the library respond properly)?

I followed the pattern of disallowing duplicate definitions of decorators but I don't see any existing tests for them. I can add them if desired.

If this is a new feature submission:

  • Has the issue had a maintainer respond to the issue and clarify that the feature is something that aligns with the goals and philosophy of the project?

Potential Problems With The Approach
None that I can think of.

const parameterMetadata = new MetadataGenerator('./tests/fixtures/controllers/hiddenController.ts').Generate();
const controller = parameterMetadata.controllers[0];

it('should generate hidden methods', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a little confused by the message 'should generate hidden methods'. Do you mean 'should generate metadata for the hidden methods even though it won't generate swagger definitions'?

Speaking of which, can you add a unit test that confirms that the swagger output does not contain the methods of the hidden controller?

I'm asking because I believe that is the primary goal of the @Hidden decorator, right?

You'll need to do those in two places:

  • For Swagger (i.e. OpenAPI v2) tests\unit\swagger\definitionsGeneration\definitions.spec.ts
  • For OpenAPI 3: tests\unit\swagger\schemaDetails3.spec.ts

Other than that, this is a great PR. Thank you for adding to the readme. Well done! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing, I can make those changes. The test message was just a victim of copy & paste from the existing @Hidden tests.

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 Swagger (i.e. OpenAPI v2) tests\unit\swagger\definitionsGeneration\definitions.spec.ts

Can you provide a little more guidance on this? I don't see any paths tests in that file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're correct. I just checked and there are not any path tests in that file. So I'm not sure where it's best to put those tests, but we do need to make sure that this decorator works for both Swagger 2.0 and Open API 3.0

expect(specHiddenMethod.paths).to.have.keys(['/Controller/normalGetMethod']);
});

it('should not contain paths for hidden controller', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

So basically the same thing you did here but with SpecGenerator2 instead. And just put it anywhere other than this file.

@ryankeener
Copy link
Contributor Author

@dgreene1 should be all set now

Copy link
Collaborator

@dgreene1 dgreene1 left a comment

Choose a reason for hiding this comment

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

Thank you! :)

@dgreene1 dgreene1 merged commit 36432f0 into lukeautry:master Oct 7, 2019
@ryankeener ryankeener deleted the hidden-controller branch October 7, 2019 16:10
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.

2 participants