Implement upsertOne() and upsertMany() for entity adapters #550

dinvlad · 2017-11-06T20:06:04Z

These methods are used to update one ore more entities
if they already exist in the state, or to add them
if they don't.

The signature of the methods is identical to that of
the corresponding update...() methods.

The commit for this PR is 9826793, others have been merged previously.

Relevant discussion: #421

EDIT: please don't merge until we agree on the signature.

Prior to this fix, collection methods in sorted_state_adapter ran in quadratic time, because they iterated over the collection with single-item mutators, each of which also running in linear time. Now, we first collect all modifications (each in constant time), then sort them in linearithmic time, and finally merge them with the existing sorted array linearly. As a result, we can improve performance of collection methods to N + M log M, where N is the number of existing items and M is the number of modifications. Single-item methods are now expressed through those for collections, still running linearly.

…_adapter Removing "holes" created by updateManyMutably() should be there instead of merge().

It should return "true" unconditionally (probably worth a test).

These methods ran in quadratic time by interating over the collection and applying a linear algorithm for each item. Now, we modify entity hashes first (each in constant time), and then update the array of their ids in linear time. As a result, all operations now occur linearly. We also simplified logic for single-item operations by expressing them through those for collections. The removal methods also apply to sorted collections (because they preserve their order).

CircleCI failed the build because includes() is not supported by its runtime yet. We use an alternative syntax to fix that.

@effect

This PR exposes a method to get metadata supplied through @effect() decorator. This method is useful for when we want to test that an effect was properly decorated/registered in the store. Related issue: ngrx#491

This commit replaces earlier function effectMetadata() with a map accessor getEffectsMetadata() and associated EffectsMetadata class. This syntax enables enables more concise access to metadata in unit tests. This is particularly nice, as we can now test both the observables and their metadata in a similar way: ``` expect(effects.someSource$).toBeObservable(expected); expect(metadata.someSource$).toEqual({ dispatch: true }); ```

Earlier version contained incorrect function name.

This change converts EffectsMetadata to a generic type that requires its keys to be keys of the effects class. We also update getEffectsMetadata() accordingly.

These methods are used to update one ore more entities if they already exist in the state, or to add them if they don't. The signature of the methods is identical to that of the corresponding update...() methods.

coveralls · 2017-11-06T20:10:12Z

Coverage increased (+0.2%) to 92.655% when pulling 9826793 on dinvlad:master into d1dba9f on ngrx:master.

dinvlad · 2017-11-10T03:10:58Z

modules/entity/src/unsorted_state_adapter.ts

+      } else {
+        added.push({
+          ...update.changes,
+          id: update.id,


This line is problematic: if selectId != entity => entity.id, this insert will fail to create an entity with the same effective id. We may need to change the signature of the method to (updates: { id: string | number; changes: T}[], state: R), per #421 (comment)

Let's make this change.

brandonroberts · 2017-11-28T02:17:24Z

@dinvlad this one will be ready to land once that changed is made and the conflicts are resolved

dinvlad · 2017-11-28T02:33:41Z

Sorry, didn't notice @MikeRyanDev's comment from earlier. Will submit soon.

bbaia · 2017-12-14T15:26:46Z

Need some help ?

dinvlad · 2017-12-15T03:51:31Z

Apologies, will make it happen this weekend.

acurrieclark · 2018-01-05T16:29:27Z

Anything I can do to assist with this?

Silthus · 2018-01-10T08:17:06Z

What is the state of this PR? Will it come soon?

jchaganti · 2018-01-11T07:32:26Z

Any updates on this PR?

icepeng · 2018-01-15T00:20:40Z

Any news?

sandangel · 2018-01-16T01:46:56Z

I have look into the code and at this time, you can do this for same behaviour:

import { Update } from '@ngrx/entity';

case '[Feature] Action':
      const updates: Update[] = action.payload
        .filter(item => item.unixTime in state.entities)
        .map(item => ({ id: item.unixTime, changes: item }));
      const inserts = action.payload.filter(item => !(item.unixTime in state.entities));
      const newState = adapter.updateMany(updates, adapter.addMany(inserts, state));
      return {
        ...newState
      };

mcblum · 2018-01-26T19:27:48Z

I certainly know everyone is busy and is working hard, but if you happen to have a rough estimate of when this may be rolled out that would be amazing. If it's coming soon I'll wait to work on my next thing, if not I'll write it by hand.

Thanks for Entities -- it's amazing!

shahafal · 2018-01-28T19:59:31Z

i like @sandangel workaround alot!

another workaround i thought of before seeing sandangel's:

function upsert<T>(entities: T[], state: State): State {
    entities.forEach(entity => {
        let key = entity['id'];
        if (key in state.entities) {
            state = adapter.updateOne({ id: key, changes: entity}, state);
        }
        else {
            state = adapter.addOne(entity, state);
        }
    });
    return state;
}

enjoy.

peterbsmyth · 2018-01-30T14:47:53Z

ngRx is MIT license. @dinvlad may take more time to finish this. Open source is open source. So I can make a new PR for this with the requested changes.

mcblum · 2018-01-30T15:55:14Z

@peterbsmith2 I actually forked the pull request in order to try and fix it but I couldn't figure out two things: 1) how do I push the changes back (dumb, I know) or do I push changes to my repo and then open a new pull request and 2) I was (indirectly) in dialog with @MikeRyanDev who explained that the upsert* methods needed to handle changes like the update* methods do to allow for circumstances where the app wrote a dummy id to the entity until save and now the id is actually changing after a successful save.

I understand the idea but couldn't actually figure out what I was supposed to do :) In my branch is also a new Upsert type that accepts T rather than Partial<t>.

mcblum · 2018-01-30T16:04:59Z

@shahafal thanks for that function. one thing I did that not only assists with this issue but any others that might come up is I made a DataFacade abstract class that can be extended to give access to a bunch of common methods that retrieve state. It gives me a way to use that facade anywhere to do things like if exists($entity) and then the base class method knows what to do with that. Even when upsert is full implemented it does seem like you can gain a lot by wrapping the entity adapter.

dinvlad · 2018-01-30T16:31:29Z

Hi folks, sorry for being silent on this and taking a long time to re-visit it. I've actually moved away from Angular to Vue (and possibly Vuex), but if everyone is happy with the signature (updates: { id: string | number; changes: T}[], state: R) then I can get to it shortly.

bbaia · 2018-01-30T18:45:18Z

There's a rebase to do first!

sandangel · 2018-01-31T21:45:17Z

@dinvlad there is an Update type you can import from @ngrx/entity as described in #670 and has been merged in #675

shahafal · 2018-02-02T12:54:50Z

@dinvlad i went over your pull request and i have a question:
in upsert we pass list of entities to be added if missing or updates if exists.
in your implementation - if an entity already exists will it get updated even if it is the same as the original element?
if the answer is yes - this raises a performance question, as entities that have no changes would get new references and reload in components with OnPush change detection strategy.

i am writing this as a comment on the pull request code (suggesting a clarifying test) too so it wont get missed.

shahafal · 2018-02-02T12:58:29Z

modules/entity/spec/sorted_state_adapter.spec.ts

+        },
+      },
+    });
+  });
 });


in upsert we pass list of entities to be added if missing or updates if exists.
in your implementation - if an entity already exists will it get updated even if it is the same as the original element?
if the answer is yes - this raises a performance question, as entities that have no changes would get new references and reload in components with OnPush change detection strategy.

anyway whatever the answer is - perhaps a test that clarifies it would be a good idea:
so if an existing entity gets an update with no changes -
a) if the functionality is to recreate it the test will show that a new entity was created.
b) if the functionality is to ignore it the test will show that no new entity was created.

MikeRyanDev · 2018-02-04T04:38:24Z

I'm going to go ahead and close this. @dinvlad or anyone else is welcome to open up a new PR!

dinvlad · 2018-02-06T01:24:47Z

Sounds good @MikeRyanDev, @sandangel is more motivated and his PR LGTM.

Denis Loginov added 13 commits September 29, 2017 02:55

Remove updateManyMutably() functionality from merge() in sorted_state…

6d2999c

…_adapter Removing "holes" created by updateManyMutably() should be there instead of merge().

Fix return value from addAllMutably() in sorted_state_adapter

ee9067c

It should return "true" unconditionally (probably worth a test).

Fix for unsupported includes() in unsorted_state_adapter

8efba70

CircleCI failed the build because includes() is not supported by its runtime yet. We use an alternative syntax to fix that.

Expose effectMetadata() useful in testing

2b9cc50

This PR exposes a method to get metadata supplied through @effect() decorator. This method is useful for when we want to test that an effect was properly decorated/registered in the store. Related issue: ngrx#491

Merge branch 'master' of https://github.com/ngrx/platform

848e94f

Merge branch 'master' of github.com:ngrx/platform

ba2eaad

Fix docs for getEffectsMetadata()

0673d12

Earlier version contained incorrect function name.

Add key type checking for EffectsMetadata

1bc563e

This change converts EffectsMetadata to a generic type that requires its keys to be keys of the effects class. We also update getEffectsMetadata() accordingly.

Merge https://github.com/ngrx/platform

cb0bee9

Implement upsertOne() and upsertMany()

9826793

These methods are used to update one ore more entities if they already exist in the state, or to add them if they don't. The signature of the methods is identical to that of the corresponding update...() methods.

dinvlad commented Nov 10, 2017

View reviewed changes

brandonroberts assigned MikeRyanDev Nov 13, 2017

brandonroberts requested a review from MikeRyanDev November 13, 2017 03:34

bbaia mentioned this pull request Nov 20, 2017

fix(Entity): updateOne/updateMany should not change ids state on existing entity #581

Merged

brandonroberts mentioned this pull request Nov 22, 2017

Entity: Document upsert methods #592

Closed

shahafal reviewed Feb 2, 2018

View reviewed changes

MikeRyanDev closed this Feb 4, 2018

sandangel mentioned this pull request Feb 4, 2018

feat(entity): introduce upsert methods #780

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement upsertOne() and upsertMany() for entity adapters #550

Implement upsertOne() and upsertMany() for entity adapters #550

dinvlad commented Nov 6, 2017 •

edited

Loading

coveralls commented Nov 6, 2017

dinvlad Nov 10, 2017 •

edited

Loading

MikeRyanDev Nov 20, 2017

brandonroberts commented Nov 28, 2017

dinvlad commented Nov 28, 2017

bbaia commented Dec 14, 2017

dinvlad commented Dec 15, 2017

acurrieclark commented Jan 5, 2018

Silthus commented Jan 10, 2018

jchaganti commented Jan 11, 2018

icepeng commented Jan 15, 2018

sandangel commented Jan 16, 2018 •

edited

Loading

mcblum commented Jan 26, 2018

shahafal commented Jan 28, 2018 •

edited

Loading

peterbsmyth commented Jan 30, 2018

mcblum commented Jan 30, 2018

mcblum commented Jan 30, 2018

dinvlad commented Jan 30, 2018

bbaia commented Jan 30, 2018

sandangel commented Jan 31, 2018 •

edited

Loading

shahafal commented Feb 2, 2018 •

edited

Loading

shahafal Feb 2, 2018

MikeRyanDev commented Feb 4, 2018

dinvlad commented Feb 6, 2018

Implement upsertOne() and upsertMany() for entity adapters #550

Implement upsertOne() and upsertMany() for entity adapters #550

Conversation

dinvlad commented Nov 6, 2017 • edited Loading

coveralls commented Nov 6, 2017

dinvlad Nov 10, 2017 • edited Loading

Choose a reason for hiding this comment

MikeRyanDev Nov 20, 2017

Choose a reason for hiding this comment

brandonroberts commented Nov 28, 2017

dinvlad commented Nov 28, 2017

bbaia commented Dec 14, 2017

dinvlad commented Dec 15, 2017

acurrieclark commented Jan 5, 2018

Silthus commented Jan 10, 2018

jchaganti commented Jan 11, 2018

icepeng commented Jan 15, 2018

sandangel commented Jan 16, 2018 • edited Loading

mcblum commented Jan 26, 2018

shahafal commented Jan 28, 2018 • edited Loading

peterbsmyth commented Jan 30, 2018

mcblum commented Jan 30, 2018

mcblum commented Jan 30, 2018

dinvlad commented Jan 30, 2018

bbaia commented Jan 30, 2018

sandangel commented Jan 31, 2018 • edited Loading

shahafal commented Feb 2, 2018 • edited Loading

shahafal Feb 2, 2018

Choose a reason for hiding this comment

MikeRyanDev commented Feb 4, 2018

dinvlad commented Feb 6, 2018

dinvlad commented Nov 6, 2017 •

edited

Loading

dinvlad Nov 10, 2017 •

edited

Loading

sandangel commented Jan 16, 2018 •

edited

Loading

shahafal commented Jan 28, 2018 •

edited

Loading

sandangel commented Jan 31, 2018 •

edited

Loading

shahafal commented Feb 2, 2018 •

edited

Loading