search

grammyjs / storages

Storage adapters for grammY sessions.
51 stars 29 forks source link

[MongoDB] Adapter should ignore unefined fields #125

Closed roziscoding closed 2 years ago

roziscoding commented 2 years ago

When writing a session, specially when using conversations, we don't really want to write undefined values as null to the database, because conversations expects some properties to be undefined, and when they're null, they break.

Reference: https://t.me/grammyjs/73326

If it makes sense, we could pass ignoreUndefined: true to the updateOne method, which would solve this problem.

Although, I don't know if it really makes sense, since we'll be changing the behavior of the adapter for inumerous cases exclusevely because of the conversations case, so maybe we should give the user an option to turn this on when installing the adapter, which would make them aware of this behavior, and could be enabled only when the user actually needs it to behave this way. What do you think?

KnorpelSenf commented 2 years ago

The fact that the conversations plugin relies on undefined being stored correct is a problem of the conversations plugin, and with oson , we already have a fix that will be brought to the plugin soon.

Nonetheless, I don't think that this adapter should convert undefined to null, but rather be consistent with JSON.stringify which strips all undefined values. What do you think?

I'm not sure if this fix would be breaking a lot of setups or not, but I don't really see how that could happen.

roziscoding commented 2 years ago

I don't like the conversion from undefined to null either, but it is the default behavior not only for the node/Deno driver, but for mongodb itself, AFAIK, so I don't know if we should change that.

Regarding breaking setups, I don't know of any cases that should break, but I assume they can exist, so I think this would be a breaking change anyway, since it messes with what will or will not be stored. We could take the flag approach and make it the default behavior in the next major, or we could release anew major now with this change.

KnorpelSenf commented 2 years ago

but it is the default behavior not only for the node/Deno driver, but for mongodb itself

So if we should not changes this, then what is there to do here? Do you mean that we should let people opt-in?

roziscoding commented 2 years ago

Considering oson should solve this, I don't think there's much to do, you're right. People who need to use conversations with mongodb (like me) can just use a custom adapter untill conversations is refactored to use oson.

Although, in the future, it might be interesting to expose a way for uses to pass options to the db calls, but that's another discussion.

I'll close this, since it's really not necessary.

KnorpelSenf commented 1 year ago

I have a feeling that this will break with multi sessions. They do not support removing properties from the session, but only assigning undefined to them.

Hence, any plugin that wants to work with multi sessions will set undefined values on the session. This will lead to delete operations.

Now, if single sessions are used with the same code, we will end up with undefined values again, as no delete operations are used.

Can you confirm that this happens with the conversations plugin on v1.1.0?