sequiturs/bookshelf-advanced-serialization

Plugin for Bookshelf.js supporting advanced serialization behavior

bookshelf-advanced-serialization's Issues

Allow incomplete specification of context designations for a given table, with sane defaults

If contextDesignator does not return a designation, we could default to:
- for ensureRelationsLoaded, an empty array
- for contextSpecificVisibleProperties, the visible properties determined by the role

Currently, if the caller uses context designations in their specification of ensureRelationsLoaded or contextSpecificVisibleProperties, those context designations must be complete in the sense of covering all possible models with the given table name (otherwise an error is thrown, cf.

bookshelf-advanced-serialization/lib/index.js

Line 429 in 94afe94

if (!Array.isArray(contextSpecificVisibleProperties)) {

,

bookshelf-advanced-serialization/lib/index.js

Line 501 in 94afe94

if (!Array.isArray(relationNames)) {

).

Serializing relations fetched with collections

    Venue
        .forge({}, {
            accessor: req.user
        })
        .fetchAll({
            withRelated: ['bars']
        })
        .then(venues => {
            return venues.toJSON();
        })
        .then(res.json.bind(res));

The accessor has sufficient privileges to see the related 'bars' model, however, the bar does not appear in the serialization. However, the following code does return the related bars.

    Venue
        .forge({}, {
            accessor: req.user
        })
        .fetchAll({
            withRelated: ['bars']
        })
        .then(venues => Promise.all(venues.map(v => v.related('bars')
            .toJSON())))
        .then(res.json.bind(res))

Any insights on why the related model wouldn't make it into the serialization?

Thanks

(I've really enjoyed using this package, btw; it's refreshingly well-documented.)

Only load relations that are in the final list of visible properties

This would be more efficient than the way we currently do it, where we load whatever relations the caller specified in ensureRelationsLoaded and then remove them if they're not in the list of contextSpecificVisibleFields.

Or we could potentially throw a sanity error to tell the caller that they instructed to load a relation which they also instructed not to be visible.

Webpage down

The url is returning {"error":"Server error."}

`ensureRelationsLoaded` when used with a collection should yield 1 join, not n joins

Currently ensureRelationsLoaded is evaluated only as part of model.toJSON(). That means each model in a collection will have its own JOIN query executed to load the specified relation. This means n JOINs, where n is the length of the collection. This is inefficient. ensureRelationsLoaded should make use of collection.load(), so that only 1 JOIN is performed.

Make handling of `contextSpecificVisibleFields` completely analogous to handling of `ensureRelationsLoaded`

This means making contextSpecificVisibleFields support indexing merely by tableName, with another level of indexing by designation optional.

Support for create/update/destroy?

Nice work on this, thank you!

Do you have plans to handle writes as well, or just keep the scope of the project to reads?

It seems like you use this in production already -- how does Sequiturs currently handle access control for writes?

Consider serializing a clone that does not have unnecessary relations, rather than deleting relations from the original model

Support passing `accessor` option to Collection constructor

Currently we only support passing the accessor option to the Model constructor.

Warn if `shallow:true` and `ensureRelationsLoaded` options are both passed to `toJSON`

When both options are passed, shallow: true should take priority since it is a standard Bookshelf option. Furthermore it should cause all ensureRelationsLoaded behavior to be avoided, since there's no reason to do unnecessary work loading relations that aren't going to be serialized--unless plugin option ensureRelationsVisibleAndInvisible is passed.

Use consistent terminology

Use "properties" instead of "fields". "properties" refers to the properties of the serialized object, which in Bookshelf's terminology are known as attributes and/or relations. There's no way to distinguish attributes from relations on a serialized object, so the more generic "properties" term is more appropriate.

Consider whether there are better names for accessor, roleDeterminer, rolesToVisibleFields, contextSpecificVisibleFields, ensureRelationsLoaded, _accessedAsRelationChain, evaluator.

evaluator should probably be renamed to contextDesignator.