Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix PUTing documents with If-None-Match when non-existent #104

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

raucao
Copy link
Member

@raucao raucao commented Mar 9, 2023

I found a pretty glaring bug when I tried to restore documents via rs-backup for development on my local machine:

The code currently does not discern between If-Match and If-None-Match in both the GET and PUT functions. It will just grab whichever header is set as the "version" and treat both headers the same way. When restoring a backup, and thus sending an ETAG in If-None-Match for a non-existent document (i.e. "create or update this document if there isn't one that matches this ETAG"), Armadietto will thus respond with a 412 conflict status.

This changeset fixes the bug, but I have not looked at the tests for it. I would appreciate if a more regular contributor could add the respective tests, since I currently don't have more time to spend on this. Thank you.

@raucao raucao requested review from DougReeder and thornjad March 9, 2023 05:07
@DougReeder
Copy link
Contributor

Yeah, I thought that was odd when I saw that code, but I didn't explore further. It appear to assume only one of those will be used with a given verb.

@DougReeder
Copy link
Contributor

It seems we have tests that storage.js passes arguments to the store, but not tests that the store properly handles If-Match and If-None-Match. So I've added some, in branch https://github.com/remotestorage/armadietto/tree/test-if-none-match

If you pull that branch and go back to commit 17a7cf7 , you can see our existing code handles If-Match with an ETag and If-None-Match with *. It doesn't handle If-None-Match with an ETag, as the signature for the put method doesn't allow put to distinguish If-Match with an ETag vs. If-None-Match with an ETag, as @raucao noted.

Unfortunately, the tests show commit b578ded fixes If-None-Match with an ETag, at the expense of If-Match with an ETag and If-None-Match with *. and a number of other tests as well.

There will need to be some changes before we can merge this.

Copy link
Contributor

@DougReeder DougReeder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems we have tests that storage.js passes arguments to the store, but not tests that the store properly handles If-Match and If-None-Match. So I've added some, in branch https://github.com/remotestorage/armadietto/tree/test-if-none-match

If you pull that branch and go back to commit 17a7cf7 , you can see our existing code handles If-Match with an ETag and If-None-Match with *. It doesn't handle If-None-Match with an ETag, as the signature for the put method doesn't allow the method to distinguish If-Match with an ETag vs. If-None-Match with an ETag.

Unfortunately, the tests show commit b578ded fixes If-None-Match with an ETag, at the expense of If-Match with an ETag and If-None-Match with *. and a number of other tests as well.

There will need to be some changes before we can merge this.

@silverbucket silverbucket self-requested a review December 29, 2023 23:26
@raucao
Copy link
Member Author

raucao commented Feb 7, 2024

Just a friendly FYI that this bug still exists (AFAIK), and prevents rs-backup from restoring backups.

However, I have no intention of adding more commits to this PR, since I'm the maintainer of a different server implementation myself (Liquor Cabinet + Akkounts), and I have no free capacity to get more familiar with the codebase here.

@DougReeder
Copy link
Contributor

This is fixed in the modular server; can we close this PR in favor of that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants