Server: Resolves #16132: Publishing/syncing an old note (serialized before the trash feature) throws Missing property "deleted_time"

[Rygaa]

Resolves #16132

My change was simple I just made sure old notes with no deleted_time field don't crash the trash/read-only check. A missing deleted_time now means "not in trash" instead of throwing.

I also added two tests. Both use an old note that has no deleted_time field:

• Test 1: the read-only check does not throw and returns false.
• Test 2: itemIsInTrash does not throw and returns false.

[laurent22]

But also why is the introduction of the notebook publishing feature causing this bug? Is there maybe a code path that shouldn't exist, or that could be tweaked? What exactly is making it throw here?

The problem with silently ignoring items without a deleted_time property is that the caller may forget to include that property, and in that case we would actually want to throw. Basically it would be good to better understand what is happening before making the code less strict

[Rygaa]

But also why is the introduction of the notebook publishing feature causing this bug? Is there maybe a code path that shouldn't exist, or that could be tweaked? What exactly is making it throw here?

The problem with silently ignoring items without a deleted_time property is that the caller may forget to include that property, and in that case we would actually want to throw. Basically it would be good to better understand what is happening before making the code less strict

In #15276, I called itemIsInTrash which wasn't used anywhere else before in packages/server Which caused the issue

A note object reaches itemIsInTrash in 2 ways: rebuilt from stored content itemToJoplinItem JSON.parse(content), or loaded from the DB.

• Rebuilt from content: old notes have no deleted_time. We can default deleted_time to 0 at rebuild time.
• Loaded from DB: notes.deleted_time defaults to 0, so the key is always there. If it is missing we should throw

So I keep the checkObjectHasProperties guard for the DB case, and just fix the content case by defaulting the value there

[laurent22]

So is there a better solution than what you are currently proposing?

[Rygaa]

So is there a better solution than what you are currently proposing?

I spoke with Henry today and we come to the conclusion that it is indeed better to keep the guard as it was before but just make sure to default deleted_time to 0 during content rebuilt.

I pushed the new changes

[coderabbitai]

.

[personalizedrefrigerator]

Mobile, Desktop, Cli: Resolves #16132: Publishing/syncing an old note (serialized before the trash feature) throws Missing property "deleted_time" - #16175

It might be better to use the "Server" prefix, so that this change shows up in the server changelog, rather than the client application changelogs.

[Rygaa]

Mobile, Desktop, Cli: Resolves #16132: Publishing/syncing an old note (serialized before the trash feature) throws Missing property "deleted_time" - #16175

It might be better to use the "Server" prefix, so that this change shows up in the server changelog, rather than the client application changelogs.

Indeed the title was more adapted to my initial fix. I updated it