Guaranteed Non-Nullability of Fields vs Nullable Fields for cross-references

[macrozone]

Describe the bug

reviews resolver might crash because author is non-nullable. But in reality, its possible that an author cannot be resolved (force-deleted or whatever)

To Reproduce

happened to us on a customer project

Expected behavior

I think author should be nullable or unchained guarantees that something is always returned and the resolver never crashes

[pozylon]

Related: https://github.com/unchainedshop/unchained/issues/323

Currently, no mutation in Unchained will lead to User deletion so having author non-nullable is safe. I will outline how we want to progress on that matter in #323. We will prioritize user deletion feature because of said project.

[macrozone]

@pozylon problem is that data is already in a broken state and its impossible to use review.author therefore. The resolver needs to either return something no matter what, or be nullable.

Even if you try to always anonymize users, database manipulation / imports / syncs might lead to these situations. mongodb cannot enforce always an existing relation, so maybe the graphql schema should reflect that. Its easier to deal with a nullable resolver then it is to deal with a resolver that might break depending on database state.

[macrozone]

in my opinion its the wrong approach to set everything to non-nullable, believing that unchained will never leave the database inconsistent. Mongodb does not guareantee consistency on fields, so there are just too many ways how this can break.

having non-nullable fields without a guarantee that the data is non-null leads to hard errors on clients, which is something that has to bee avoided, because its impossible to work around that.

If there is one inconsistency somewhere, you can't use the author resolver at all.

[pozylon]

The reason why that happened on that customer project was a special custom extension that was directly mingling with the db. So to solve it the custom extension should be accountable of leaving a clean state.

While I agree that there is no guarantee through the DB because MongoDB lacks referential integrity, the consequence of setting all fields that query referenced documents to optional types also has its downsides because you need to guard all those fields on the client.

I think this is maybe a conceptional issue but it's not worth changing it yet because this only happened one or two times in 3 years, so I'm renaming the ticket and tag it as wontfix for the moment.

[macrozone]

still happens with unchained 1.x.

now it even happens if the product exists in the underlying database.

Error: Cannot return null for non-nullable field ProductReview.product.',

i checked using the aggregation pipeline that for each productReview there is a product:

db.getCollection("product_reviews").aggregate(

    // Pipeline
    [
        // Stage 1
        {
            $lookup: // Equality Match
            {
                from: "products",
                localField: "productId",
                foreignField: "_id",
                as: "products"
            }

        },

        // Stage 2
        {
            $addFields: {
                numProducts: {$size: "$products"}

            }
        },

        // Stage 3
        {
            $match: {
               numProducts: 1

            }
        }
    ],

    // Options
    {

    }

);

ProductReview.product should be nullable or unchained should guarantee that there is always a product to return. If unchained cannot guarantee that, the field has to be nullable.

EDIT: with bysecting the failing graphql request "productReviews(...) (using limit and offset...) i found out that the query fails if it contains a review for a product that has "status" : "DELETED",

there is an another problem by the way that if using productReviews without a sort argument, but using limit and offset, you will get inconsistent results. No default sort is used and the underlying database might not guarantee any specific order. The query always have to use a default sort order, this is true for all paginated queries.

[pozylon]

thanks for helping to solve the productReviews case, i'm looking into the sort thing

[macrozone]

still broken on my project. Unchained should switch to nullable fields if it can't guarantee that there is always a product there

[pozylon]

just extend the schema and make it nullable for the moment @macrozone:

extend ProductReview {
   product: Product
}

[pozylon]

I think i'm finally convinced @macrozone I just saw it myself ...