Add attributes whitelist

[confuser]

Currently removing attributes is available via omitAttrs, but there isn't an opposite to include only certain attributes.

Use case is for ACLs, to only allow certain fields to show to users, depending on their roles. Whilst this could be done at the model level via columns/select/visibility plugin, it's common to require non-visible fields for computations.

jsonapi-serializer has an attributes option which allows this functionality, but it's only available at the time of creating a mapping instance via new Mapper.Bookshelf. It'd be great if this could be exposed as an option within the map function.

[ShadowManu]

The API request sounds reasonable to me, but lets see what the team think of it.

Regarding implementation details, the jsonapi-serializer attrributes option is being generated from the model/collection and the corresponding options, like the blacklisting omitAttrs. We'll have to adhere to existing APIs so to not break anything.

Regarding specific implementation, the logic is being condensed at this part of the source code. I would like to ask too, what kind of API addition to consider plausible.

Even more, we'll keep being open to Pull Requests! What a better way to have something implemented than taking business in your hands right? But here we are to discuss more details and give guidance and feedback if needed ;).

[chamini2]

Hey @confuser, this certainly looks like a good idea, given that we already have omitAttrs, it makes sense. But I would recommend using Bookshelf's visibility plugin since the resources may be included with a ?include=... query param and then you would have to list the whitelist considering those too.

A better idea is to maintain the options of the mapper for foreign keys and keep the restrictions of attributes dependent of roles to the Bookshelf plugin.

[confuser]

@ShadowManu I'm afraid I've not worked with TypeScript before, looks a little daunting, but I'll see if I get some time this week to give it a go.

@chamini2 The visibility plugin doesn't offer fine grain control that's needed for ACLs. It's only hide/show X fields, not only show X,Y,Z fields if user has role A. I've already built the logic to handle which attributes should show, the requested option at the time of mapping is the only missing piece.

[ShadowManu]

Use this previous comment as reference for my advice on Typescript. tl;dr: don't be afraid, write JavaScript, we are here if you struggle with the TypeScript.

[confuser]

What's the preference for the property option? I'd suggest attributes, as it uses the same terminology as jsonapi-serializer

[chamini2]

Well, we could merge omitAttrs and this new option a single one attributes or attrs.

I imagine this one to be used like:

mapper.map('type', data, { attributes: { omit: ['one', 'two'], include: ['three', 'four'] } })
mapper.map('type', data, { attributes: ['three', 'four'] }) // refers to `include` sub-option by default

What do you guys think?

We would have to define what to do in cases like {attributes: { omit: ['one'], include: ['one'] }}

[ShadowManu]

LGTM

[confuser]

@chamini2 I'm afraid I don't see why a breaking change would be needed to add this in. The options already follows json-serializer terminology, it would be somewhat more sensible to remain along those lines.

[chamini2]

@confuser, we do not design our API to go along with the serializer's. And since we are in beta versions, there should be no problem breaking, we specifically stayed in beta to decide the final API for version 1.0.0.

The reasoning behind merging the two options in a single one would be because they are conflicting options anyways (i.e. what should we do with an attribute if it's present in both options), so they might as well be together.

We are still open to suggestions or a discussion about it! I'm just expressing my opinion.