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

Maintaining order in relationships #83

Closed
opsb opened this issue Jan 31, 2015 · 13 comments
Closed

Maintaining order in relationships #83

opsb opened this issue Jan 31, 2015 · 13 comments

Comments

@opsb
Copy link
Contributor

opsb commented Jan 31, 2015

I need to store the order that objects appear in a hasMany. I optimistically tried to just work with the natural order but it looks like this isn't possible (I'm using localstorage and the storage format appears to use a map rather than an array). It looks like my best option is to maintain the order in a separate field (have a position field on each object is another option but seems like it would be a lot more chatty in terms of transforms). @dgeb have you given any thought to this yet?

@opsb
Copy link
Contributor Author

opsb commented Feb 1, 2015

@dgeb, been looking at how arrays are treated by json patch, and in particular json pointer. According to https://tools.ietf.org/html/rfc6902#appendix-A.7 json arrays should be referenced using an index e.g.

/task-list/92664ad9-2bdc-44a7-a747-df195a7adb45/__rel/tasks/1

but currently orbit.js uses an id instead of the index e.g.

/task-list/92664ad9-2bdc-44a7-a747-df195a7adb45/__rel/tasks/5f70d429-c198-4b22-b506-7c4e1191e3aa

Is there any particular reason (perhaps performance) that you implemented it this way or was it simply a case of being the easiest way to get the project up and running? In any case would you consider changing orbit.js to use array indexes? (I'm happy to have a look at this)

@opsb
Copy link
Contributor Author

opsb commented Feb 1, 2015

I see now that orbit.js actually does this already, https://github.com/orbitjs/orbit.js/blob/master/test/tests/orbit/unit/lib/diffs-test.js#L44. I guess the Source isn't treating the relationships as arrays when it generates the ops.

@opsb
Copy link
Contributor Author

opsb commented Feb 1, 2015

Ok, so orbit.js handles updates to arrays and https://github.com/orbitjs/ember-orbit/blob/master/lib/ember-orbit/links/has-many-array.js#L23 has the necessary positional information. It looks like ember-orbit's store needs to be updated somehow to be aware of changes to array links. It's not obvious what the cleanest way to achieve this is yet though.

@dgeb
Copy link
Member

dgeb commented Feb 1, 2015

Orbit handles collections as unordered sets (rather than ordered arrays) by default because it is by far the more common and practical approach to managing collections in a distributed environment.

However, I agree that there is a place for tracking ordered arrays. I think it is cleanest to implement when the collection is updated as a whole, but I can also imagine valid use cases for positional updates of individual items.

As you've noted, JSON Patch supports these use cases, and so does Orbit's Document class and diffs and eq method.

This hasn't been too high on my priority list, but it's something that I'd like Orbit to handle eventually. I believe that we need a flag in the schema to indicate that a collection is ordered (e.g. ordered). This could be combined with the actsAsSet flag to cover all the possible cases. I haven't thought through all the implementation details, but it definitely seems feasible. Obviously, all the source and cache implementations would need additional logic to handle these flags.

@opsb please let me know if you decide to take this on and I can try to provide guidance if needed. If this seems daunting, you could certainly maintain a position field on ordered collections (as you mentioned) and we could work on a more elegant solution later.

@opsb
Copy link
Contributor Author

opsb commented Feb 1, 2015

@dgeb I'm having a crack at the moment. My basic approach has been to start at https://github.com/orbitjs/ember-orbit/blob/master/lib/ember-orbit/links/has-many-array.js#L34 and work my way down the call stack adding removeArrayLink alongside removeLink etc. I've done this for the add... methods as well. I've successfully got the store updating the arrays, now working on the lookup logic (it's expecting id => true instead of position => id). Once I've got a working scenario I'll put back and think about how to implement it in a way that can configured as you suggest (the ordered flag).

@dgeb
Copy link
Member

dgeb commented Feb 1, 2015

@opsb Sounds good. It's possible that addLink and removeLink could be modified to handle ordered links (e.g. perhaps an integer could be treated as an array index, and a string as an id). But keeping the logic separate for now probably makes it easier to reason about.

@opsb
Copy link
Contributor Author

opsb commented Feb 1, 2015

@dgeb yeah, that's certainly something I'm considering, but as you say at the moment I'm just trying to get a working scenario so I can figure out a good implementation. Having said that I did find the overloaded methods a little harder to follow (i.e. hasOne and hasMany using the same methods) so it might be clearer to separate those out.

@opsb
Copy link
Contributor Author

opsb commented Feb 2, 2015

Having thought about this some more it would seem that replacing the entire link using updateLink would be more appropriate. It means the interface to source remains unchanged and there's less risk of the schema going out of sync with the ember-orbit association. My plan is to implement the following for an ordered hasMany on Source:

updateLink: copy over the array of ids into the link
addLink: raise error
removeLink: raise error
findLink: return array of ids
findLinked: resolve records using array of ids

Does this cover all the methods that will need updating? Does this seem like the best approach to you?

@dgeb
Copy link
Member

dgeb commented Feb 2, 2015

@opsb I think it's fine to still support addLink and removeLink, since these are convenience methods that convert a developer's requests into transforms. The important point is that, when a transform is applied to a link marked with the actsAsSet flag, then its didTransform event should reflect that the entire set was replaced. Does that make sense?

Besides that one aspect, everything else sounds great. Thanks for checking in 👍

@opsb
Copy link
Contributor Author

opsb commented Feb 2, 2015

@dgeb I'm treating actsAsSet and ordered as different variations on hasMany i.e. I'm leaving the actsAsSet code alone and adding new code branches at various places to handle storing the ordered hasMany in an array. Is this what you're expecting (I wasn't sure from what you mentioned about actsAsSet)?

@opsb
Copy link
Contributor Author

opsb commented Feb 2, 2015

This also means that actsAsSet and ordered are mutually exclusive flags in the schema so I'll throw an exception if they're both specified.

@dgeb
Copy link
Member

dgeb commented Feb 2, 2015

This also means that actsAsSet and ordered are mutually exclusive flags in the schema so I'll throw an exception if they're both specified.

I think this is fine for now, although I can imagine them being used together or independently (as I hinted above).

@opsb
Copy link
Contributor Author

opsb commented May 30, 2016

The rethink branch introduced one way data-flow which makes management of ordering far simpler. Now that it's been merged into master we'll revisit it in that context.

@opsb opsb closed this as completed May 30, 2016
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

2 participants