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

New Records in a Child Session Don't Stay There #49

Closed
ginty opened this issue Aug 2, 2013 · 7 comments
Closed

New Records in a Child Session Don't Stay There #49

ginty opened this issue Aug 2, 2013 · 7 comments

Comments

@ginty
Copy link

ginty commented Aug 2, 2013

It has always seemed annoying to have to clear up un-saved models when leaving a create route and I thought that the child session capabilities for EPF would simplify this considerably.

e.g. with code like this the new object could be created upon entering the route, and if the route is left via any means other than the save action the child session and it's contained object would just get forgotten about and garbage collected.

model: ->
  @childSession = @session.newSession()
  @childSession.create(App.Foo)

events:
  save: ->
     @childSession.flush().then ->
        # Leave upon successful save

However this doesn't work because every time the child is flushed the new record is put into the parent session before trying to persist it. If this fails then an unsaved record is left in the parent session and not isolated to the child as I would have expected.

Is this a bug or are my expectations wrong?

Thanks!

@jasonkriss
Copy link
Contributor

This is indeed a bug. Take a look at #22. It's an update rather than create there, but it's the same issue.

@ghempton
Copy link
Contributor

ghempton commented Aug 3, 2013

What do you guys think is the desired behavior here:

  1. Wait until the child session flush() is successful before updating the parent.
  2. Optimistically update the parent when there is a flush() and rollback if it fails.
  3. Keep the existing behavior and assume there is only isolation up until the first flush() call.

One of the main reasons for keeping things isolated is to support cancel behavior. If a flush is called it becomes clear the intent is not to cancel.

@jasonkriss thoughts?

@ginty
Copy link
Author

ginty commented Aug 4, 2013

I think it should be either (1) or (2), and would prefer (1).

(3) is flawed because it allows errors from the child session to bleed into the main session which does not provide the level of encapsulation that you would expect from the child session

It should definitely be able to handle the conceptual case where a user tries to do something that saves to the server -> it then fails for whatever reason and gives the user feedback -> the user then decides to abandon the change.

I prefer (1) for the example case of a user updating their profile. Say they make some changes that are deemed to be invalid upon saving, the username is already taken for example, in this case you would not want the username attached to the main session to be updated to the new value.

Also by not updating the parent until the flush is successful it will mean that you can still cancel the change late in the day by simply walking away from the child session.

Thanks.

@jasonkriss
Copy link
Contributor

I tend to agree with @ginty for the most part here. Although, I guess not familiar enough with the internals yet to understand if there would be any advantages to (3). The example of a user changing their username only to find out it is already taken, is definitely the kind of thing that happens quite a bit. In that case, it's crucial to get back the original value. It's obvious how this would work in (1) and (2), but how would it work in (3)?

@ghempton are you leaning towards a particular solution?

@ghempton
Copy link
Contributor

ghempton commented Aug 6, 2013

I think I might ultimately support multiple isolation levels. I can see the argument for all 3 cases. The reason why #3 still appeals to me is that it allows for optimistically assuming the updates succeed (which I think is important). This also is related to @jasonkriss example in #51 when editing a profile inline.

@sapientpants
Copy link

I would like to propose a variation on (1). Since there is not an actual transaction, on flush(), only those records that are successfully persisted are moved to the parent session. Any records that failed persisting remain in the child session to either be fixed and persisted, or abandoned.

At any rate, I like (3) the least as it pollutes the parent session whenever a flush() fails. (2) just seems like a more complicated variation of (1).

Just my two cents.

@ghempton
Copy link
Contributor

This has since been cleared up. See #59

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

No branches or pull requests

4 participants