fix: duplicate key exception #907
Conversation
Signed-off-by: alperozturk <[email protected]>
jancborchardt
requested review from
AndyScherzinger,
David-Development,
stefan-niedermann and
tobiasKaminsky
korelstar
left a comment
korelstar
left a comment
There was a problem hiding this comment.
Can confirm, that this fixes nextcloud/notes-android#2848. However, I'm not sure, if this could cause problems for other HTTP headers.
|
Hopefully this doesn't break anything elsewhere. FWIW, this PR is what RFC 2616 & 9110 say to do with duplicate headers. https://www.rfc-editor.org/rfc/rfc9110#name-field-lines-and-combined-fi |
|
Hello, However, the code just joins any header values with a ", " separator in between. I don't know whether the app relies on any of these headers (or will do so in the future), but you might rather want to join distinct values only to avoid issues. In my case it was caused by two |
Signed-off-by: alperozturk <[email protected]>
alperozturk96
added
3. to review
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
| final String value = header.getValue(); | ||
|
|
||
| // Create a unique key for name:value combination | ||
| final String key = name.toLowerCase() + ":" + value; |
There was a problem hiding this comment.
Thanks!
This could fix downstream issues like e.g. nextcloud/notes-android#2531
There was a problem hiding this comment.
Thanks!
This could fix downstream issues like e.g. nextcloud/notes-android#2531
I don't think so. It doesn't actually store the headers with the lowercase key, only uses that for deduplicating them.
For that issue, I did comment on #796 with a likely solution, but I didn't manage to build Notes against my test version to find out.
|
Hey. Thank you for your messages. I’ve updated the PR with tests. Since I wasn’t able to reproduce the issue on my side, could you please test the latest version from here ? As you can see from the tests, I think the expected values appear to be reasonable, but I’d appreciate your confirmation. |
|
I've just tested that build, syncing works correctly again and newly created notes show up without a refresh (nextcloud/notes-android#2850) |
korelstar
left a comment
korelstar
left a comment
There was a problem hiding this comment.
Thanks! Latest updates still fix nextcloud/notes-android#2848. I would be happy to see this released soon :-)
andrewille
left a comment
andrewille
left a comment
There was a problem hiding this comment.
Looks good to me and works well. :)
Fixes
nextcloud/notes-android#2848