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

Annotation with slashes sync problem crashes taskwarrior #12

Closed
Tranquility opened this issue Feb 10, 2019 · 13 comments
Closed

Annotation with slashes sync problem crashes taskwarrior #12

Tranquility opened this issue Feb 10, 2019 · 13 comments
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@Tranquility
Copy link

I added a url (http://...) to a task as a annotation. After changing and syncing the task with foreground Taskwarrior crashed. After debugging it a little bit I found out that foreground converted http:\/\/ to http:\\\\. This seems to happen only with annotations.

@bgregos bgregos added the bug Something isn't working label Feb 12, 2019
@bgregos
Copy link
Owner

bgregos commented Feb 12, 2019

Did Foreground crash or did Taskwarrior itself crash? In any case, I'll investigate as soon as I get a chance.

@Tranquility
Copy link
Author

Taskwarrior crashes, when I run task sync.

@bgregos bgregos added the good first issue Good for newcomers label Dec 13, 2020
@jumper149
Copy link

jumper149 commented May 22, 2021

I had a very similar experience: I just marked a task as done (in foreground) and the next time I synced with the regular taskwarrior executable I got a Segmentation fault.

{"description":"Bremsbelaege wechseln","uuid":"9f335227-17a5-409d-bf71-a672a0c2856c","project":"bike","status":"completed","priority":"L","modified":"20210522T231757Z","end":"20210522T231757Z","entry":"20210414T120108Z","tags":[],"annotations":"[{\"entry\":\"20210414T120114Z\",\"description\":\"was kaufen? bestellt.\"},{\"entry\":\"20210515T190607Z\",\"description\":\"anbauen!\"}]"}
75eb2bc5-fb7d-4d9c-aa39-97df0735f693

You should look at the backslashes in the annotations field!!

@bradyt
Copy link

bradyt commented May 23, 2021

It looks like any edit to a task in Foreground, changes the value of an existing annotation from a list to a string.

At least, that's what it looks like watching the outgoing and incoming changes from the perspective of a Taskserver's logs.

I imagine this issue happens as Foreground reads a task from the device into memory, then edits and saves the task back to device storage.

@bgregos
Copy link
Owner

bgregos commented Jun 1, 2021

I suspect this problem comes from how Foreground saves/syncs tasks.

Tasks are sent to/from the task server in JSON format, and these tasks are saved to local storage in JSON format as well. Somewhere in this process of converting JSON to java objects and back again, these slashes are being added to escape special characters in a Java string when they really shouldn't be. These slashes then get sent back to the taskserver and synced to another client where it causes the crash.

This post has more info on what's likely happening:
https://stackoverflow.com/questions/51510733/how-to-avoid-backslashes-in-gson-jsonobject

@bgregos
Copy link
Owner

bgregos commented Jun 1, 2021

The best place to look is in Task.kt, which includes the toJson() and fromJson() functions that convert a Task (Foreground's internal representation) to JSON that's saved and synced. Any field that Foreground doesn't have a native field for (name, due date, etc) is stored in a map of Strings called others. Since this nests JSON objects in a map of strings that itself is serialized to JSON, I bet this is the culprit.

I'll attempt to get a fix up sometime soon, but my next few weeks are going to be rather busy. I'd be happy to accept a PR if anyone wants to give it a try.

@bradyt
Copy link

bradyt commented Aug 19, 2021

@bgregos what if you change val others: Map<String, String> to val others: String, by encoding and decoding the intermediate result between Map and String? That might result in a significant refactor though. Maybe put more of the "standard/core attributes" in the explicit part of representation?

This is basically what I recently ended up doing, I have intermediate steps to help represent the UDA field as a single string, even though it represents a map. I was having difficulty because not all values in the map were strings. But I could easily encode the whole map to a string anyways.

Or maybe another idea, what if you encode the values in the map back to json strings? In pseudocode, something like,

for(extra in task.others){
    out.putOpt(extra.key, json.encode(extra.value))
}
...
for (key in obj.keys()){
    others[key] = json.decode(unescape(obj.getString(key)))
}

Most likely, I have misunderstood the code.

Here's some code and tests in case it gives some ideas:

@bradyt
Copy link

bradyt commented Aug 24, 2021

Hmm, the strategy I described above, applying that to annotations would be more of a stopgap, to avoid the errors in other clients. I guess the longer term solution would be to actually represent the annotations as a list of maps or list of a new class Annotation's.

Do you have unit tests to check how your task representation works round-trip on json strings? That might be a simple way to assure there's no issues with all the Taskwarrior core/standard attributes or UDAs. If you want a simple way to get json strings tasks to test, I believe task export gives similar json strings, the only difference is you get a json list of maps, separated by commas, whereas the taskd payload has json string tasks separated by newlines. But I think the json string tasks are similar, except export includes id and urgency.

That is, taskd payload is like,

{"description":"foo",...}
{"description":"bar",...}
...
{"description":"baz",...}

Whereas task export is like,

[
{"id":1,"description":"foo",...},
{"id",2,"description":"bar",...},
...,
{"id",0,"description":"baz",...}
]

@bgregos
Copy link
Owner

bgregos commented Aug 25, 2021

There aren't any unit tests for this at the moment, but it's a good idea to add them. The recent major refactor/rewrite that was released as version 1.4 should make it much easier for unit tests to be written.

@linuxcaffe
Copy link

In testing future annotation round-trips, please include newline characters, as some folks enjoy multi-line annotations.

@morganmay
Copy link

I hope this gets fixed; I would really like to try Foreground, but many of my existing tasks have annotations with URLs and file paths that contain slashes, and I don't want to corrupt my data.

@bgregos
Copy link
Owner

bgregos commented Sep 22, 2021

Good news! I think I've finally managed to fix this bug. I have the work pushed to the annotation-slashes branch now and if everything goes smoothly it will be going out in the next Foreground release (most likely a minor version with this fix and some Play Store housekeeping).

I would be very appreciative if someone could pull down that branch and do some additional testing to make sure it's fixed and that no other breakages happened.

@bgregos
Copy link
Owner

bgregos commented Nov 16, 2021

I just released Foreground 1.5.2 which includes the fix for this issue. It's available now via the Releases tab and it's rolling out through Google Play and F-Droid.

@bgregos bgregos closed this as completed Nov 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

6 participants