Allow overriding returnUri and returnBuffer with params

[dasantonym]

Summary

I'd like to be able to conditionally choose if a URI or a Buffer should be returned on some occasions, overriding the default global service setting.

This way, I can use a hook to set returnBuffer and returnUri by setting these props on context.params.

[claustres]

It seems to make sense, my only suggestion would be to use query parameters instead so that it can also be used from the frontend side without additional hook, just like we do for the version.

Would you mind adding appropriate tests as well ?

Thanks

[dasantonym]

Yes, you're right, that would be better. I'll make the changes and will add the tests as well on Monday. Thanks!

[claustres]

I think you can remove the !== undefined part in your conditions. Otherwise providing a null returnUri option for instance will cause it to be considered true, which does not seem to be what the user expect IMHO.

[dasantonym]

Good call, thanks! But for my case it might be more complicated. My default setting is true and I want to override per request. Now that the setting for both params can come both from a hook (various js types) or via query (always string), I need to check for common falsy values (0, null, undefined), too, don't you think?

I added a method for that to util.js, not sure if this is overkill...

[claustres]

Yes it seems to me overkill and complex for what you want to achieve. There are two use cases:

1. The returnUri option is set to true in the service. Then you'd like to be able to deactivate it on request by providing a false query parameter value, otherwise the service defaults applies.
2. The returnUri option is set to false in the service. Then you'd like to be able to activate it on request by providing a true query parameter value, otherwise the service defaults applies.

So it seems to me we only have to check if there is a query param and if so use the value of the query param, otherwise use the value of the service defaults, probably something like:

let returnUri = this.returnUri
if (query.hasOwnProperty('returnUri')) returnUri = query.returnUri
...

No need to check for different types as now it is only a matter of if returnUri can be coerced to true or false in the subsequent code. What do you think ?

PS Of course the approach is similar for the returnBuffer option.

[dasantonym]

I totally agree on use case 2, but in case 1 i supply false as a query param and it comes in as a string 'false'. So then returnUri is set to a string which is still truthy.

How and where would you suggest to coerce it to a boolean value then?

[claustres]

Usually required type conversion is performed by hooks with Feathers. But in this simple case we can manage it directly, e.g. this should handle most possible values (true, True, false, False, 0, 1): if (typeof returnUri === 'string') returnUri = Boolean(JSON.parse(returnUri.toLowerCase())).

I am however a bit surprised the param is not converted to a boolean, have a look to the basic provided example and it works "as is". If values were not converted to the correct types the matching in the DB would not work. It seems to me that express or socket.io handle this transparently, converting strings from/to JSON objects.

By the way, adding tests will make this more clear.

[dasantonym]

Ah, thanks for clarifying this! I'll look into why I am getting a string there, but I can definitely drop the extra function then.

And yes, I pledge to finally add the tests!

[dasantonym]

So, I reverted the last additions and added some tests.

But now that I wanted to add tests for the service operations from the client, I now see that disabling returnUri does not work since it is coming in as a string. The test fails, but this is expected behaviour as returnUri needs to be parsed to bool in a user-defined hook (as suggested here: https://docs.feathersjs.com/api/databases/querying.html#querying).

Should I add that test anyway testing for the expected behaviour? Should I add a hook to the test?

[claustres]

We could probably provide a hook performing the required conversions if we'd like to follow the feathers recommendations. Your test will then simply validate that a service with this hook works as expected. You should create a hooks.js file and export the hook in the index.

[dasantonym]

I must admit that I am fairly new to the way feathers handles things. Please have a look if this is alright or if there should be any more changes.

[claustres]

Looks like a pretty good job for a newcomer. Just check if there is any console.log() remaining, it seems to me there is at least one in the tests. Then we will be ready to merge, thanks again for this.

[dasantonym]

Thanks a lot for all the help! And a bit of a crash course in feathers architecture, too.