Skip to content
This repository has been archived by the owner on Feb 26, 2021. It is now read-only.

Denormalize userId onto Todos collection #17

Closed
stubailo opened this issue Nov 6, 2015 · 16 comments
Closed

Denormalize userId onto Todos collection #17

stubailo opened this issue Nov 6, 2015 · 16 comments
Assignees

Comments

@stubailo
Copy link
Contributor

stubailo commented Nov 6, 2015

So that we can do better authorization and avoid fetching the List for every todo operation.

@tmeasday
Copy link
Contributor

tmeasday commented Nov 9, 2015

Check it out

@tmeasday tmeasday assigned stubailo and unassigned tmeasday Nov 9, 2015
@stubailo
Copy link
Contributor Author

It seems like this would be super hard to implement correctly in general, since you need to parse the update modifier. Is there a way to do it somehow so that it doesn't involve writing a ton of code for all the different cases of update?

@tmeasday
Copy link
Contributor

Yeah, it's kind of the same problem as allow/deny.

I'm not really sure what the best approach is.

Would the following logic work?:

const targetModifier = {};
const changingMods = _.each(modifier, (changes, mod) => {
  if (_.has(changes, 'userId')) {
    targetModifier[mod] = {userId: changes[userId]};
  }
});

That would work for the $set and $unset cases anyway. Probably weird edge cases around $push & friends though.

An alternative would be to look at the source document after the update. I think the trick there is to detect if the field changes without doing unnecessary lookups.

@stubailo
Copy link
Contributor Author

Would it maybe be OK to do one more lookup for each insert that does denormalization?

@tmeasday
Copy link
Contributor

If that's possible? This is on an update to the source collection -- I don't think we want to do an unnecessary lookup if we can help it -- so we'd want to figure out from the modifier if a look up is necessary -- which is kind of an easier version of the original problem..

@stubailo
Copy link
Contributor Author

Hmm.. I feel like analyzing update operators is like the opposite of how I'd rather be spending my time.

Is it really that bad to just do the denormalization in the methods directly rather than inside update? Perhaps there's a way to just warn people if they forgot to do denormalization, or test it easily, or something?

@tmeasday
Copy link
Contributor

Yeah the problem with putting it in methods is that it's spread all over the place. But maybe an intermediate solution is for the method to call Denormalizer.update(...) or something?

@stubailo
Copy link
Contributor Author

Yeah and it could perhaps pass the new value of the field, since that will be clear from the method body.

@tmeasday
Copy link
Contributor

This works for me as a solution for now anyway. Want me to change it?

@stubailo
Copy link
Contributor Author

yeah, although I'm not sure if Denormalizer.update is the right thing? Is it like Denormalizer.<method name>?

@tmeasday
Copy link
Contributor

Oh, so what I meant was something like

Lists.methods.makePrivate = new Method({
  run({ listId }) {
    ...

    Lists.update(listId, {
      $set: { userId: this.userId }
    });
    Lists.userIdDenormalizer.set(listId, this.userId);
  }
});

@tmeasday
Copy link
Contributor

(An conceivably if another method set the userId on a list, it would also call Lists.userIdDenormalizer.set

@stubailo
Copy link
Contributor Author

Yeah that looks awesome.

@stubailo
Copy link
Contributor Author

So I guess the burden on the developer would be:

  1. Whenever you write a new method, check to see if any denormalizers need to be added
  2. Whenever you add a new denormalizer, check all of the methods to see if you need to add it there

Shouldn't be too bad, I think?

@tmeasday
Copy link
Contributor

Yeah. I mean it's not as nice as putting it on the mutator, but it's not bad. And if someone makes a bulletproof denormalizer package, then I guess we can go back to it.

@tmeasday
Copy link
Contributor

The method pattern means at least you know where to look!

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

No branches or pull requests

2 participants