Allow truthy values for the GraphQL replication deletedFlag field.

[nirvdrum]

This PR contains:

• IMPROVED typings
• A BUGFIX
• A NEW FEATURE

Describe the problem you have without this PR

I'm replicating from a GraphQL store in front of a PostgreSQL database. Handling soft deletion in relational databases by storing a deletedAt|deleted_at timestamp column rather than a boolean flag is a common practice. While I could wrap the whole thing with a view to expose a boolean flag, it would be handy if I could use this timestamp directly.

I thought maybe RxDB handled truthy values for the deletedFlag by default. This seemed to work fine with PouchDB, but when I tried LokiJS, I ran into a confusing problem. When data was pulled from the GraphQL service, it would show in the web app immediately. But, when the web app was reloaded, the data wouldn't show. I tracked that down to queries having the where('_deleted').eq(false) selector appended by default. In my case, the _deleted value was a nullable timestamp, so either there was a string present with an ISO 8601 datetime string or the JavaScript value null. Neither of those value is false, so Loki would fail to find any documents.

I think supporting truthy values here would allow RxDB to work with a more diverse set of GraphQL servers. If someone needs more advanced rules (e.g., maybe they use an enum and values are always "true" in JavaScript) then a more comprehensive solution would be needed, maybe in the modifier callback. But, allowing truthy values here supports real world cases that were not well-supported before.

Moreover, I think this PR closes a usability gap with RxDB. By allowing non-Boolean values to be stored in the _deleted property of the store document, there's a confusing situation where freshly pulled documents show up in query results because they already exist in memory but fail to load from persisted storage because of the malformed _deleted value.

Todos

• Tests
• Documentation
• Typings
• Changelog

I added documentation for this change, but I was unable to run GitBook locally. I tried all of the NPM scripts in package.json and each time I got the following:

> npm run docs:serve

> rxdb@11.3.0 docs:serve
> gitbook serve docs-src

Installing GitBook 3.2.3
/home/nirvdrum/dev/workspaces/rxdb/node_modules/npm/node_modules/graceful-fs/polyfills.js:287
      if (cb) cb.apply(this, arguments)
                 ^

TypeError: cb.apply is not a function
    at /home/nirvdrum/dev/workspaces/rxdb/node_modules/npm/node_modules/graceful-fs/polyfills.js:287:18
    at FSReqCallback.oncomplete (node:fs:199:5)

I'll try to figure out what is going on there. But, the point is I wrote the docs with a local Markdown editor plugin but was not able to verify the build documentation looks good.

[pubkey]

For me it looks like this would create a burden for every future implementation of RxStorage.
Imagine you have to implement a RxStorage based on PostgreSQL. Then you would have to create a field of type "whatever is truthy in JavaScript".

CouchDB does only allow boolean values for _deleted, see here
PouchDB might allow other values, but to be honest, PouchDB's behavior is completely strange at many points, I do not think they allow all truthy values intentional.

Gitbook is not maintained, this is why I run it in an old node.js version in the CI. Related

[nirvdrum]

I'm not sure what the burden is. Could you please elaborate?

The change I made is in the GraphQL replication layer, so as far as I can tell, it doesn't matter what the underlying storage mechanism is. There's already an implicit requirement in the replication layer that the deletedAt flag corresponds to a Boolean value. If you don't give it a Boolean value, queries won't work consistently and leads to some very confusing behavior. All I've done is ensure the value is actually Boolean. It imposes no additional requirements on the storage layer. Unless I've done something wrong, the storage layer should never going to see anything other than a Boolean value. Maybe my test is insufficient?

[nirvdrum]

Okay. I took another look and it's rx-storage-loki that is enforcing a Boolean value on the _deleted flag in this case. If the idea is that you want the deletedFlag to be whatever type the GraphQL response is, would you accept a PR that translates to a Boolean value at the loki storage layer where deletedValue is supplied? I did it at the GraphQL replication layer because I thought the Boolean flag was a hard requirement based on the documentation. But, I'm happy to move the conversion wherever makes most sense.

[pubkey]

Hi @nirvdrum
I am a bit ashamed. I totally misunderstood the proposed change because I did not properly read the commit.
The main change is doing const deletedValue = !!doc[this.deletedFlag];.
Having the truthy-to-boolean conversion in the GraphQL layer is ok for me.

Merged, thank you for your work.
I will do a release tomorrow.

[nirvdrum]

@pubkey No worries. Thanks for taking a look. I'm still building a mental model of RxDB, so I appreciate you taking a critical look at what I've been proposing. I try to ensure backwards-compatibility in everything, but there are invariably cases I'm unaware of. Thanks for the merge. If you're looking to release just for me, you don't need to rush. I'm running a fork at the moment until I can finish up the work on #3630.

[nirvdrum]

CouchDB does only allow boolean values for _deleted, see here
PouchDB might allow other values, but to be honest, PouchDB's behavior is completely strange at many points, I do not think they allow all truthy values intentional.

To follow-up on this part, I was mistaken about what PouchDB was doing. I forgot that I had run into an issue where adding a sort option to a query in PouchDB added in deleted documents. To deal with that, I had wrapped all of my queries with where('_deleted').ne(true), so if the value was undefined it would be treated the same as false. This is a situation where where('_deleted').eq(false) and where('_deleted').ne(true) are not equivalent operations. Fortunately, only Boolean values should get stored going forward so the semantic difference should not matter.