Node-restify: Add HTTP COPY support for Restify 4.x

[arcaderob]

Feature Request

Add HTTP COPY support to Restify 4.x - http://www.restpatterns.org/HTTP_Methods/COPY

Use Case

I am adding an asset duplication endpoint to an API. I have the need for the Restify module to support the COPY verb for proper routing and the usage of the destination and depth headers.

Example API

Frontend
An example of the request that is made in AngularJS:

$http({
    method: 'COPY',
    url: '/someUrl'`,
    headers: {
        Authorization: {auth token},
        Destination: {absolute URI}
        Depth: {required depth}
    }
})
.then(function successCallback(response) {
    // handle success response from API
},
function errorCallback(response) {
    // handle error response from API
});

Backend
An example of the route in NodeJS:

server.copy({
    name: 'api-copy-endpoint',
    path: 'someUrl',
    version: '1.0.0'
},
authenticateAuthHeader(),
validateUrlParams(),
controller.copyFn());

Are you willing and able to implement this?

I have created a PR adding the COPY verb to the collection of accepted HTTP methods and included tests for the code.

PR: https://github.com/restify/node-restify/pull/1371

[arcaderob]

1371

[retrohacker]

Hey @arcaderob!

Thank you for taking the time to open such a thorough feature request, and for including an accompanying PR. I believe we have tossed around the idea of custom HTTP verbs before, (i.e. https://github.com/restify/node-restify/issues/576)

I'm :+1: on supporting arbitrary verbs, though I believe that only the official HTTP verbs should be hard-coded. Is there a way we could make this generic?

[retrohacker]

Also, apologies on the radio silence, I was OOO for the past 2 weeks. Just getting back to working through the open issues :-)

[arcaderob]

Hey @retrohacker

No worries on the delay.

I am not 100% certain if I understand what you mean by making the request handling generic. Are you suggesting that we implement the ability to specify a verb and the module have a way to handle any type of verb that it is sent? So the user can specify any verb they like?

Would the user be able to specify anything that they like? Such as a made up verb like FOO :

$http({
    method: 'FOO',
    url: '/someUrl'`
});

OR
would we want to stick to a collection of acceptable verbs such as the ones listed here?

I would be willing to have a look into this with some more direction from you and what you would like for your module.

Thanks for taking the time to look into this request and PR.

[retrohacker]

Exactly that. So we should have a set of core supported verbs (I would think HTTP/1.1) which we have convenience methods for, and then we provide a way for users to extend restify with their own custom verbs (i.e. WebDAV) through something along the lines of a server.addRoute.

(addRoute is more of a scarecrow example than a proposal for an actual method for the API.)

[arcaderob]

Hey @retrohacker

Thanks for the input.

Sorry for the delay in my response. This sounds doable and I am willing to look into it. I will see what I can do after hours in the next little bit and see if I can't put something together.

[sean3z]

Allowing consumers to add support for their own verbs (while core remains hardcoded) certainly sounds like the best route to go. I'd be interested in knowing more technical insight on the implementation of this potentially new api though 😃

For example:

let server = restify.createServer();
server.addRoute('copy'); // using @retrohacker scarecrow example
server.copy('/route', (req, res, next) => {});

1. Should there now be a specific error thrown if the consumer tried to access a verb that had not yet been added (rather than the generic TypeError: server.copy is not a function)?
2. What happens if you invoke addRoute('copy') more than once? Error/Ignore?
3. Would there need to be a way to determine if a new verb has already been added? e.x:
   js if (!server.routeExists('copy')) { server.addRoute('copy'); // other things... }
4. Would there additionally need to be an option to removeRoute()? What could this break? 😛

5. Would addRoute() invokers need to need to pass a middleware-like method to specify what qualifies as that supplied verb? e.g.:

   server.addRoute('copy', (req, res, next) => {
     if (req.method !== 'copy' && !req.headers.destination) return res.send(400);
     next();
   });
   
   server.copy('/route', (req, res, next) => {});
   

   • This could be handy as it allows greater flexibility for consumers but potentially dangerous as it can create disconnect between the actual request method and server.[verb] route definition.

Sorry for the trip down (a probably unnecessary) rabbit hole, just thinking out loud :)

[retrohacker]

Hey @sean3z! Thanks for engaging in the conversation, it really got me thinking about this feature request! :smile:

My scarecrow was along the lines of:

server.addRoute('copy', '/route', (req, res, next) => {});

That being said, I _really_ like the idea of sever.addVerb(). It would keep the rest of the API 100% consistent and there is the added bonus of this being able to leverage the current implementation pretty easily. The PR implementing that would only be a few lines of code plus tests. I'm going to run with this idea to answer your questions

Should there now be a specific error thrown if the consumer tried to access a verb that had not yet been added (rather than the generic TypeError: server.copy is not a function)?

I don't believe that is possible in JavaScript, it would require having a default function execute when property that doesn't exist is accessed off of the Server object. Though I may be misunderstanding the question.

What happens if you invoke addRoute('copy') more than once? Error/Ignore?

I would prefer the function be idempotent. You should be able to call it an infinite number of times, if the verb already exists it would be a no-op.

Would there need to be a way to determine if a new verb has already been added?

If the function is idempotent, the provided example is redundant (since you could just add the route). Alternatively you should be able to do something like if(typeof server.copy === 'function') to check if it exists. We could provide a sugar wrapper for this later if needed. That being said, I can't think of a reason where you would want to _dynamically_ add an HTTP verb that isn't abusing the HTTP protocol, so idempotence should be plenty.

Would there additionally need to be an option to removeRoute()? What could this break?

I can't think of a case where this would be necessary. Not having handlers for a verb would be essentially the same as removing the verb entirely from my understanding. We could add it later on if there is a compelling argument for it.

Would addRoute() invokers need to need to pass a middleware-like method to specify what qualifies as that supplied verb?

I think this would be an _alternative_ implementation to what is suggested here, and possibly a more universal solution. Adding a middleware tier for routing logic would allow a user to create named middleware stacks and have a router direct requests accordingly. I'd be worried about the performance overhead of a middleware pattern here though since every request would have to be passed through the routing middleware stack before falling back to restify's default logic. That is a non-trivial amount of overhead on an extremely hot path just for routing...

[retrohacker]

I created an example implementation of this at #1399.

Some initial concerns:

1. How do we handle name collisions with existing properties/functions on server? i.e. address and listen prevent adding ADDRESS and LISTEN verbs respectively.
2. Implicit handling of server.del -> DELETE and server.opts -> OPTIONS may be surprising, and prevents adding a DEL and OPTS HTTP verb.
3. Are these verbs or methods? We are introducing an inconsitency with the existing naming conventions of the codebase. That being said addVerb seems more appropriate compared to addMethod, but addVerb(method) is confusing.

[sean3z]

Excellent points @retrohacker! There's a lot of information here, so I'll try to reply to the most pressing parts (in no particular order) 😄

I don't believe that is possible in JavaScript, it would require having a default function execute when property that doesn't exist is accessed off of the Server object. Though I may be misunderstanding the question.

It's possible with Proxy (specifically handler.get()) but, yes, probably overkill in this case :)

Are these verbs or methods? We are introducing an inconsitency with the existing naming conventions of the codebase.

Good point! When referencing code, "method" has multiple meanings however that is the proper term when addressing HTTP "verbs". I'll try to use "request method" hereon to eliminate any unintentional ambiguity.

How do we handle name collisions with existing properties/functions on server? i.e. address and listen prevent adding ADDRESS and LISTEN verbs respectively.

Do we really need to make custom server.[request method] available with the below implementation? If the request method and the route are defined together, I'm not sure if there's an additional need for accessing that request method directly off of the server object.

server.addRoute('copy', '/route', (req, res, next) => {});

Implicit handling of server.del -> DELETE and server.opts -> OPTIONS may be surprising, and prevents adding a DEL and OPTS HTTP verb.

As I understand it, del and opts are just shorthands for DELETE and OPTIONS (probably for contextual reasons). There's a internal "handler" for OPTIONS errors but, other than that - I don't believe restify does (should?) have any internal implementation (plugins excluded) for these two request methods.

A slightly different approach?

I'm not an expert on the codebase so forgive my ignorance but, Server.prototype[method] accepts an object. What if we exposed Server.prototype.custom (or another generic naming convention) for the following-like implementation?

let options = {
  path: '/route', // looking at the codebase, i think this works today
  method: 'COPY'  // support would need to be added for this
}

server.custom(options, (req, res, next) => {})

Outside of perhaps some minor server.js logic rewiring, I'm unsure what additional effort would be required other than changing the follow to:

- opts.method = method.toUpperCase();
+ opts.method = (opts.method || method).toUpperCase();

This allows the Server prototype to remain intact without any quirky consumer alterations and potential collisions.

Thoughts?

[retrohacker]

Woah, this fell off my radar, sorry @sean3z.

Do we really need to make custom server.[request method] available with the below implementation? If the request method and the route are defined together, I'm not sure if there's an additional need for accessing that request method directly off of the server object.

I agree, and taking your approach simplifies things a lot :tada:

I really like your proposal, and think it is in a place that I would be happy to see a PR from anyone interested! :heart:

CC:// @yunong and @DonutEspresso if either of you are interested in reviewing the proposal

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stale[bot]]

This issue has been automatically closed as stale because it has not had recent activity.