Modifying a story can easily lead to data loss

[sbleon]

I think this is because follower_ids is "excluded by default" when retrieving the story. The gem thinks the story has no followers, and when you save the story, it sends follower_ids: []. This may also be a problem with other fields that are excluded by default.

[sbleon]

Workaround: use TrackerApi::Endpoints::Story#update and send only those fields you know you want to change.

[sbleon]

Issue title updated to reflect severity of problem.

[forest]

Good find @sbleon. If you are interested in making a fix for this it would be greatly appreciated.

It should be easy to add a failing spec. Then I think the right place for a fix is the UpdateRepresenter that is responsible for serializing the object to json. https://github.com/dashofcode/tracker_api/blob/master/lib/tracker_api/resources/story.rb#L39

Should be able to exclude the follower_ids field if it is empty.

[sbleon]

I think that would work for follower_ids (since it should never be empty
because the requestor is always a follower). However, that approach might
not work for other attributes that are “excluded by default”. I’d consider
a couple of approaches:

1. Ensure that all story attributes are fetched when stories are loaded.
   I’m assuming the API lets you do this somehow.
2. For updates, only send attributes that were either fetched or set via
   mutator methods.

I suspect that 1 may be easier to implement since you would not have to
mess with the representers at all. But 2 would be more efficient over the
wire for both fetches and updates.

On Fri, Feb 5, 2016 at 12:18 PM, Forest [email protected] wrote:

Good find @sbleon https://github.com/sbleon. If you are interested in
making a fix for this it would be greatly appreciated.

It should be easy to add a failing spec. Then I think the right place for
a fix is the UpdateRepresenter that is responsible for serializing the
object to json.
https://github.com/dashofcode/tracker_api/blob/master/lib/tracker_api/resources/story.rb#L39

Should be able to exclude the follower_ids field if it is empty.

—
Reply to this email directly or view it on GitHub
#62 (comment)
.

[forest]

The problem with 1 is that there is a very nice fields param that lets you get just what you want and/or have fields included that aren't normally sent. This is a really powerful feature that can greatly reduce the data size of the response.

I wonder if it is better to force people to be specific about what they want updated.

story.name = "New Name"
story.labels << TrackerApi::Resources::Label.new(name: "New Label")
story.save(:name, :labels)

Or to deprecate TrackerApi::Resources::Story#save and make people use TrackerApi::Endpoints::Story#update which as you mention works and gives full control.

Deprecating save makes the client simpler for sure.

Thoughts?

[sbleon]

I think that providing Story#save makes the gem easier to use, so I
wouldn’t want to scrap it altogether. It would be better if we can make it
behave as expected. What about my #2 with a twist? We could keep track of
which attributes are dirty, and pass only dirty attributes when calling
#save. That would make #save behave like the
ActiveRecord/ActiveResource objects most of us are used to.

I’m not sure if this is possible with Representable. I looked quickly at
that project’s docs but I didn’t see anything obvious. This post may be
relevant:
http://stackoverflow.com/questions/28483145/how-to-dynamically-add-properties-to-a-roar-representer

On Fri, Feb 5, 2016 at 3:36 PM, Forest [email protected] wrote:

The problem with 1 is that there is a very nice fields param that lets
you get just what you want and/or have fields included that aren't normally
sent. This is a really powerful feature that can greatly reduce the data
size of the response.

I wonder if it is better to force people to be specific about what they
want updated.

story.name = "New Name"
story.labels << TrackerApi::Resources::Label.new(name: "New Label")
story.save(:name, :labels)

Or to deprecate TrackerApi::Resources::Story#save and make people use
TrackerApi::Endpoints::Story#update which as you mention works and gives
full control.

Deprecating save makes the client simpler for sure.

Thoughts?

—
Reply to this email directly or view it on GitHub
#62 (comment)
.

[forest]

I agree that providing Story#save makes the gem nicer to use, but if it exposed other issues then I'm not sure it is worth it.

My first implementation of Story#save was with dirty tracking. It required a bunch of hacky code to get it to work with Virtus and ended up creating other issues. I decided to scrap the idea. Maybe it is worth another try.

I don't have a ton of time, but I'll do some more thinking and prototyping on this. If we can come up with a nice pattern it will make it easy to add update functionality to other Resources easier.

[forest]

I'll give dirty tracking another try this afternoon.

I found a Virtus extension that looks better than my previous attempt.
https://github.com/mr-dxdy/virtus-dirty_attribute

[forest]

@sbleon check out #65. I had to make one breaking API change, but I think it is worth it.

This commit 27599e7 has the main changes if you want to focus in.