getJoin should throw error on failure to retrieve joined documents

[primitive-type]

Since the get command throws an error rather than returning null, I think getJoin should exhibit similar behavior in cases where the OtherModel document in the relation does not exist (i.e., the leftKey field is present on the Model document but does not reference any valid OtherModel document). I'm not sure whether the error thrown should be DocumentNotFound or ValidationError, but one of those seems appropriate to me.

In cases where there are no joined documents to retrieve (no references on the Model document to an OtherModel document), I think the current behavior is fine. This issue only refers to cases where there are invalid references present.

Does this make sense?

[tlvenn]

Hi @primitive-type,

Thanks for the clarification, I understand a lot better what you are after. Given that RethinkDB does not provide referential integrity, you would like Thinky to react in some ways when it detects that obviously something is wrong as for example, a document referencing another document by its id but that document does not exist.

I wonder how costly it would be to check that and what would be the ideal feedback Thinky should give. If the impact is marginal, it could be handy that Thinky warns the dev somehow that a relationship is not maintained/cleaned properly...

[primitive-type]

@Tlvenn Yes, that's what I'm talking about. I think the feedback would ideally just be a DocumentNotFound error being thrown in those cases, and the message could specify which faulty relation was the culprit.

Currently I'm trying to perform some checks along these lines manually using the document EventEmitter methods, but I haven't had much luck with that approach yet.

[neumino]

Hum, there's currently no way to say that a document has joined document that are required. One way to hack around for now is to define your own validator on the model.

[primitive-type]

Ah! I didn't even think of the field validator functions. I will try that now.

In the meantime, this issue isn't describing a way to make a joined document required, but rather to ensure that references to joined documents don't point to nonexistent documents. In other words, if there is a reference key to a joined document on a given document, that reference key should point to an existing document. If the reference key doesn't exist, no behavior would change.

[neumino]

Oh thanks for the precision, I didn't think about the case where a foreign key lead to nothing. That probably deserve an error (or at least an option to return an error).

[primitive-type]

This is what I came up with in my hapi application to ensure foreign keys lead to actual documents (using the co library):

// Ensure that a related document exists in the database
server.method('validateRelation', function (id, modelName, next) {
    return co(function* () {

        // Get the thinky model from the application
        var model = server.plugins[modelName].model;

        // null means the joined document is not required and is not present in this case
        if (id !== null) {
            yield model.get(id).run();
        }
    }).then(next).catch(function () {
        throw new Errors.ValidationError(
            modelName + ' document with id ' + id + ' does not exist.'
        );
    });
});

Since the individual field validator method on thinky schemas seems to be synchronous, I instead call the above method in a model's validate post hook for each of the model's relations, like this:

Model.post('validate', function(next) {
    Promise.all([
        server.methods.validateRelation(this.foreignKeyA, 'relatedModelNameA'),
        server.methods.validateRelation(this.foreignKeyB, 'relatedModelNameB')
    ]).then(function () {
        next();
    }).catch(next);
});

So every time a new document is created with foreign keys, those foreign keys are checked to make sure that they reference actual documents of the related model. After further thought, it probably makes more sense to do this when creating or saving a document rather than retrieving the document. But I could still see a potential use case for doing it in getJoin as well.