[WIP] feat(openapi-v2): add decorator for head op

[b-admike]

Add sugar function for head operation.

Connect to #691

Checklist

• npm test passes on your machine
• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide
• Related API Documentation was updated
• Affected artifact templates in packages/cli were updated (N/A?)
• Affected example projects in packages/example-* were updated (N/A?)

[b-admike]

@slnode test please

[bajtos]

Nice 👍

[jannyHou]

lgtm 👍

[b-admike]

@slnode test please

[jannyHou]

@b-admike I am not quite familiar with these test cases, but I feel the spec for a function that has a return arg is different than the one that doesn't have a return...so I am not sure how the same openrationSpec passes for both?

Maybe the code with your change is correct. It depends on does operationSpec contains a response object.

[b-admike]

@jannyHou I think these tests are for asserting that the correct operation object is mapped to its corresponding controller method. Yeah I think the response object in operationSpec is the same throughout the tests.

[jannyHou]

hmm...I think if this helper function calls withStringResponse then we probably could not omit the return in those controller functions.

[b-admike]

Discussed this with Janny, but for posterity, we do not have any assertions in the tests for the response object or the actual response, and thus having a return statement with a string for the controller methods doesn't have any effect.

[raymondfeng]

@b-admike Can you explain why you removed all return values for greet?

[b-admike]

@raymondfeng it's because we weren't using them in the tests and it increased our code coverage. See my discussion with Janny above.

[raymondfeng]

Tests are examples for some developers to learn and follow. I would rather not sacrifice them for artificial coverage numbers. It's better to use /* istanbul ignore next */ to instruct the tool. See https://github.com/gotwarlost/istanbul/blob/master/ignoring-code-for-coverage.md.

[b-admike]

I mainly got rid of them because I thought they didn't serve any purpose. The coverage increase was just a bonus :). But I don't mind reverting those changes and using the ignore comments.

[shimks]

+1 on Raymond's comment, otherwise LGTM

[raymondfeng]

@b-admike It does serve some purposes to show the usage of our decorators and when they apply. For example, GET usually expects to see a response from the return value. Developers might be confused to see greet does nothing and returns nothing.

[raymondfeng]

Please revert the changes on greet and use /* istanbul ignore next*/ to ignore such sections if desired. I'm fine to let the coverage tool complain for these cases.

[virkt25]

Great addition!! Just thinking about this from a developer perspective though ... declaring an additional @Head() with a function seems like extra work. Can this decorator be nested on a function doing the @get() call but make the function just return the headers. Or perhaps we should consider a way to mark a GET route as HEAD enabled ... or we just enable it by default for all GET requests and the decorator allows header values to be set?

I'm ok with the changes. Asking this question more for the purposes of discussion before any changes are made. Reference for purpose of HEAD request: https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/HEAD

[b-admike]

@virkt25 Those are good points. I'm up for finding alternatives to this approach! Not sure how to implement what you've suggested though.

[raymondfeng]

@virkt25 Good point.

@b-admike Maybe it's not so useful to have @head any more and @get will imply. I think HEAD can be handled by the router so that requests will be routed to GET but remove the response body.

[virkt25]

I think it might be better from a performance standpoint to not execute the GET function at all rather than to remove the response body. Unnecessary delay in response to HEAD + additional hits against DB.

But we should be able to set the content-type header (json / xml / whatever) for the expected response. And for files (if static handler will be using the same router) ... we should be able to return the Content-Length header and type header appropriately as well.

[raymondfeng]

Without invoking the get, head won’t have the same list of response headers.

[virkt25]

I was just wondering if we may be able to avoid that overhead by letting users optionally specify the headers they want to pass along for a HEAD request. But if there's no other way then I'm fine with calling GET handler and dropping the body. Seems to be what Express does:

The app.get() function is automatically called for the HTTP HEAD method in addition to the GET method if app.head() was not called for the path before app.get().

[raymondfeng]

openapi-v2 is now removed.