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

[Proposal] Allow paths in ids #432

Closed
tstirrat opened this issue Sep 28, 2016 · 21 comments
Closed

[Proposal] Allow paths in ids #432

tstirrat opened this issue Sep 28, 2016 · 21 comments
Assignees
Labels

Comments

@tstirrat
Copy link
Contributor

tstirrat commented Sep 28, 2016

Proposal

Allow paths in ids e.g. project1/-abcdef so that deep data structures can be supported. Deep structures are often used in Firebase apps and are particularly useful when locking things down via security rules.

Detailed explanation

Without storing within deep paths, it is difficult to write security rules that allow listening for live changes across a subset of records. For example, if you were to have a chat app where messages were stored under the messages collection and secured so that no single user can read all messages:

// data
messages : {
  -Xyz: { from: 'user1', to: 'user2', ... },
  -Xzz: { from: 'user2', to: 'user3', ... },
}

// rules
"messages": {
  ".read": false,
  "$messageId": {
    ".read": "auth.uid == data.child('from').val() || ..."
}

This will prevent a realtime query on messages. You cannot do a query on the top level, because ".read" is false. Your current option is to store each message in a hasMany relationship under user.messages, or create some other intermediate object with hasMany links.

If we could use paths in both ids and queries:

// data
messages : {
  conversation1: {
    -Xyz: { from: 'user1', to: 'user2', conversationId: 'conversation1', ... },
    -Xzz: { from: 'user1', to: 'user2', conversationId: 'conversation1', ... },
  },
  room1: {
    -Abc: { to: 'room1', ... },
  }
}

// rules
"messages": {
  ".read": false,
  "$conversationId": {
    ".read": "root.child('rooms').child($roomId).child('members').child(auth.uid).val() == true",
}

Example API

// chat room message
const conversationId = room.get('id');

// private conversation
const conversationId = [me.get('id'), you.get('id')].sort().join('_');

// get messages via a raw firebase query
const messageQuery = this.get('firebase').child(`messages/${conversationId}`).limitToLast(100);
store.query(messageQuery).then(liveArray => {
  this.set('model', liveArray);
});

// save new message
const newId = this.get('firebase').push().key;
const message = store.createRecord('message', {
  id: `${conversationId}/${newId}`,
  roomId: conversationId,
  body: 'Hello world',
}).save(); // saves at /messages/:conversationId/:pushId

Breaking changes

The intent is for this to be backwards compatible with the existing API and data structure.

Open questions

  • How would relationships be saved in the db? with slashes, replace dots with slashes?
@mikkopaderes
Copy link
Contributor

Hmm. So if I imagine it correctly, the message model would still be created without any properties. It's just for the sake of the createRecord working. Is that correct?

Will the deep message under messages/conversation be it's own model? If so, how would the model blueprint look like for this data structure?

usersConversations: {
  user1: {
    conversation1: true
  }
}

conversations: {
  conversation1: {
    name: 'Ember',
    createdOn: 12345
  }
}

@tstirrat
Copy link
Contributor Author

tstirrat commented Oct 3, 2016

Define the model exactly the same as you normally would. Even if you are considering nesting them, you only define the model once:

// models/message.js
export default DS.Model.extend({
  body: DS.attr('string'),
  author: DS.belongsTo('user', {async: true}),
});

If you do not want to use nesting - e.g. for users, for rooms - create records like normal. They will operate the same as now.

If you need nesting for performance or security (usually both), you save the records with paths in the id:

this.store.createRecord('message', {
  id: 'conversation1/-KTA9ZViW86U9re5GD3q',
  // ...
}).save();

Ember Data will consider it just like any other record except now we will use the collection and id to find the path to the actual location in the DB e.g. messages/ + conversation1/-KTA9ZViW86U9re5GD3q.

If you were to look into the Ember inspector and look at the store, you would see a messages collection with ids in it like conversation1/-KTA9ZViW86U9re5GD3q

Does that clear up your questions?

@tstirrat
Copy link
Contributor Author

tstirrat commented Oct 3, 2016

A bit of a tangent...

Need to figure out how embedded records should be saved.. We could keep the existing functionality for now, but with the new path-aware save/retrieve it might make sense to use path based ids for embedded objects, too.

Note that we cannot use the collection + id method to find an embedded object, they are almost always nested under a different collection.

If we could put an absolute path in the ids like /users/user1/addressInfo (embedded belongsTo link to model address), or /users/user1/favorites/-KTAFmS8GBOdF3vkgTb_ (embedded hasMany link to model favorite)

If we used this absolute terminology, getting an embedded objects ref is easy.. (new Firebase(db).child(embeddedAbsolutePath)) vs the current system where we need to look up the implicit relationships.

@mikkopaderes
Copy link
Contributor

@tstirrat, sorry, I should've specified that I wanted to know how the model would look like for:

usersConversations: {
  user1: {
    conversation1: true
  }
}

The conversation1 only holds a true value, not an object that maps perfectly to an ember model. Would that simply be a model without any properties? Like this:

// models/user-conversation.js
export default DS.Model.extend({
});

And saving it would be just providing an id? Like this:

this.store.createRecord('user-conversation', {
  id: 'user1/conversation2'
}).save();

But that's just assuming that we force it to be just true, there should probably be a convention to indicate what the value would be rather than just being true all the time.

@tstirrat
Copy link
Contributor Author

tstirrat commented Oct 4, 2016

That would not be possible to map in EmberFire without hard coding like this:

// models/user-conversation.js
export default DS.Model.extend({
  conversation1: DS.attr('boolean'),
});

I assume that is not what you want, you want a dynamic list of conversations..

Is there a particular reason why you need it in that specific form?

If you need a joining-model type thing, you could do the following:

// models/user-conversation.js
export default DS.Model.extend({
  conversations: DS.belongsTo('conversation', {async: true}),
});

// output
{
  userConversations: {
    user1: {
      conversations: {
        -KTA9ZViW86U9re5GD3q: true,
      }
    }
  }
}

However this is not that much better than adding conversations into the user model.
You'd only want it out of the user model if you wanted to secure it differently (e.g. any user can see who user1 is chatting with).

The problem with relying too much on relationships ("joins"):

  • they require lots of async lookups in emberfire
  • Ember Data tries to reconcile relationships, if a user pushes bad data (e.g. the inverse key is null) it can update your relationships in a strange way on the client.
  • at the moment they have no sorting, and even if we fixed it to allow sorting, it would require a bunch of bookkeeping to ensure the order stays intact when two people edit the same relationship
  • no way to limit on relationships in Ember Data, imagine a hasMany with 5000 items and you only really need the last 20?

A good pattern to work towards is constructing the path to the data you need on the client side without looking at a relationship key. e.g. I want all messages between user1 and user2, create a conversationId using the user ids that will never change (there is an example in the first post here), then query /messages/:conversationId/ .. Another example is a series of edits to a document over time -- query on /documents/doc1/edits

This is the essence of why I am proposing a change where EmberFire will work better with deep data.

/edit emberfire->ember data

@mikkopaderes
Copy link
Contributor

mikkopaderes commented Oct 5, 2016

@tstirrat

I assume that is not what you want, you want a dynamic list of conversations..

Yes I meant a dynamic list of conversations.

no way to limit on relationships in Ember Data, imagine a hasMany with 5000 items and you only really need the last 20

Exactly, that's why in my work I avoid having hasMany relationships. Instead I flatten them out and do a native firebase call rather than going with the with the ember data way.

e.g.

If I have a model of structure like this:

// models/user
export default DS.Model.extend({
  name: DS.attr('string')
});

// models/post
export default DS.Model.extend({
  title: DS.attr('string'),
  body: DS.attr('string')
});

And a data structure of this:

{
  users: {
    user1: {
      name: 'rmmmp'
    }
  },

  userPosts: {
    user1: {
      post1: true,
      post2: true
    }
  }

  posts: {
    post1: {
      title: 'Post 1 title',
      body: 'Post 1 body'
    },

    post2: {
      title: 'Post 2 title',
      body: 'Post 2 body'
    }
  }
}

As you can see, I have no model for userPosts. Right now what I simply do is do a native Firebase query to fetch the userPosts/user1/ then do a find record for every snapshot I get (this.store.findRecord('post', 'post1') -> this.store.findRecord('post', 'post2')).

Also, userPosts is the one with deep IDs but I don't think the proposed changes would be able to handle that. Since the proposal would only solve the deep IDs problem if my structure looks like this:

// models/user
export default DS.Model.extend({
  name: DS.attr('string')
});

// models/user-post
export default DS.Model.extend({
  title: DS.attr('string'),
  body: DS.attr('string')
});

{
  users: {
    user1: {
      name: 'rmmmp'
    }
  },

  userPosts: {
    user1: {
      post1: {
        title: 'Post 1 title',
        body: 'Post 1 body'
      },

      post2: {
        title: 'Post 2 title',
        body: 'Post 2 body'
      }
    }
  }
}

Now I have no problem with the proposal. It's just that since we're here discussing it, I wanted to see if we could solve 2 problems in 1. But I'm a little bit convinced now that it's really going out of the ember way if we try to solve data structures that looks like mine.

On another note, I think it'd be pretty awesome if instead of having a store.query like this:

// get messages via a raw firebase query
const messageQuery = this.get('firebase').child(`messages/${conversationId}`).limitToLast(100);
store.query(messageQuery).then(liveArray => {
  this.set('model', liveArray);
});

We could have it like this:

this.store.query('message', {
  deepId: conversationId,
  limitToLast: 100
});

jamesdaniels added a commit that referenced this issue Oct 5, 2016
#432 (#438)

* Adding a note about security rules and linking to #432 
* Fixing the markdown link
* Addressing @tstirrat's review: fixing language and clarifying
@tstirrat
Copy link
Contributor Author

tstirrat commented Oct 6, 2016

+1 to having a path in the query.

I was also thinking we could help with your situation currently by letting the query know if you're expecting objects, or links to objects. If it was expecting links to objects you could use it for your structure above.

this.store.query('post', {
  path: `userPosts/${userId}`,
  type: 'ids', // expecting ids, not objects
  limitToLast: 100,
});

We could look for id1: true and convert this to an async object lookup for store.findRecord('post', 'id1')

edit: type is probably not the right parameter name, but you get the idea.

@mikkopaderes
Copy link
Contributor

I think type wouldn't be necessary. I guess it's safe to assume that if there's a path property inside the query, it would always be expecting IDs. At least it should be. It wouldn't make sense that the store.query is looking for a post model and then the path userPosts/${userId} possibly being a model by itself since it's an object.

But yeah, this would really be awesome and would help support the flattened data structure of Firebase in Ember. Looking forward to it. Thanks!

@tstirrat
Copy link
Contributor Author

tstirrat commented Oct 7, 2016

With the example given in the first post of this proposal you would need to query objects under a path.

@tstirrat tstirrat assigned tstirrat and unassigned jamesdaniels and tstirrat Oct 20, 2016
@brendanoh
Copy link

@tstirrat can you explain this:

With the example given in the first post of this proposal you would need to query objects under a path.

@tstirrat
Copy link
Contributor Author

@brendanoh : @rmmmp suggested that type in a query is not necessary because a query is always expecting ids.. I am suggesting that if one was to implement their data structure like in the example given at the beginning of this proposal, the query would be looking for objects rather than simply ids.

For this reason I think a query should be able to handle both scenarios.

@Luiz-N
Copy link

Luiz-N commented Oct 26, 2016

So I'm actually working on a side project where I needed to update firebase outside of ember (via lambda services) and I was doing something similar. In my app I have a user model which hasMany projects which hasMany projectDays. In this case I was creating the id for a new projectDay by joining the project id with a timestamp.

  var projectDayObj = {};
  var projectID = payload.event.project.id 
  // projectID == userID + "_projects/" + projectKey
  // assuming userId == 123 and projectKey == 098 this would look like 123_projects/098
  // and live in firebase at projects/123_projects/098
  // to create a new projectDay record i first get a timestamp
  var unixDaystamp = moment.utc().startOf('day').valueOf(); // (this could be a new random key if a diff type of model)
  // to create the projectDayId I use the same pattern as before...
  var projectDayID = projectID + "_projectDays/" + unixDaystamp;
  // projectDayID would then be  '123_projects/098_projectDays/unixDaystamp' (what you'd see in ember inspector)
  // and thus live in firebase at 'projectDays/123_projects/098_projectDays/unixDaystamp'
  // these next 3 lines set the projectDay as a child of the project
  var projectDayPath = 'projects/' + projectID + '/projectDays/' + projectDayID
  projectDayObj[projectDayPath] = true
  firebase.update(projectDayObj)
  // I can then start messing with the projectDay this way
  firebase.child('projectDays/' + projectDayID).transaction(....

This allows for situations where lets say I only have access to the user ID in my lambda function then I can still get all their projects without having to first query for all the project ids. I simply search for:

firebase.child("projects/" + userID + "_projects")

Everything still works the same on the ember data side of things.

Curious to hear any feedback on this structure and any performance/security rule implications.

@tstirrat
Copy link
Contributor Author

@Luiz-N The structure you are using is great for performance. Bucketing content (e.g. in your case you bucket into days) prevents queries from growing uncontrollably and also prevents huge amounts of data being read if someone decides they want to do a once('value').

Storing content at a location that you can reconstruct later without doing any extra lookups (as yo are doing with projects/<userId>_projects is also recommended.

The one situation where you might run into issues is sharing the information and allowing access to it via security rules. In the case of user projects, do you plan them to be only accessible by that single user? If not, then they might be much harder to find for other users.

tstirrat pushed a commit that referenced this issue Oct 26, 2016
* master: (37 commits)
  Update documentation to use query rather than find (#451)
  [DOCS] update quickstart and guide with security rules changes (#450)
  Added #448 to the changelog
  fixes #447, ServerValue.TIMESTAMP is now correctly handled (#448)
  Add code climate badges to readme
  Need to addonify the lcov.info (#446)
  Forgot to add the COVERAGE env variable needed to generate the report (#445)
  Allowing ember-data beta to be an allowed failure (#444)
  Code Climate integration (#443)
  Adding additional resources to the Guide (#440)
  [firebase-release] Removed change log and reset repo after 2.0.4 release
  [firebase-release] Updated EmberFire to 2.0.4
  Update changelog.txt
  FastBoot support (#439)
  Documentation change: adding a note about security rules and linking to #432 (#438)
  Documentation change: suggestion to use inverse null (#437)
  [firebase-release] Removed change log and reset repo after 2.0.3 release
  [firebase-release] Updated EmberFire to 2.0.3
  Update changelog.txt
  Add support for limit and filter query combination (#435)
  ...
@Luiz-N
Copy link

Luiz-N commented Oct 26, 2016

Thanks @tstirrat. I've just recently started reading into the security rules in depth. Since the rules cascade down couldn't I use the same rules as if it was a shallow structure? I also don't see anywhere in the docs for how to best share data (for example via an admin of sorts) in order to see another user's data.

@tstirrat
Copy link
Contributor Author

@Luiz-N you could write admins to a secure location and use root.child('secureData/admins/'+auth.uid).exists() anywhere else to enforce admin status

@jamesdaniels
Copy link
Contributor

jamesdaniels commented Oct 26, 2016

Something I thought of this AM + chatted briefly w/@tstirrat

What would you guys think of a find/query syntax in the format of {class}@{path}. Rather than tossing a Firebase Ref. E,g.:

// Looks for message objects under /different/path in the Real-time Database
this.store.find('message@different/path')

Of course this.store.find('message') would still search in the "assumed" path as it does today.

Further, we could expand the syntax to include searches "by id" with the format of {class}#{path}

// Grab 100 ids under /userPosts/${userId} and map to find('message', ...)
this.store.query('post#userPosts/${userId}', { limitToLast: 100 });

Perhaps you could even combine the two:

// Grab 100 ids under /userPosts/${userId} and map to find('post@publishedPosts', ...)
this.store.query('post@publishedPosts#userPosts/${userId}', { limitToLast: 100 })

Does that seem Ember/Firebase-ie?

@mikkopaderes
Copy link
Contributor

For me it kinda feels not ember data-ish.

Right now in ember data, the current standard for querying data as shown in the guides should be of this syntax:

this.get('store').query('person', {
  filter: {
    name: 'Peter'
  }
});

Of course, it depends on the adapter if they'll handle that syntax. But I think it would be nice if emberfire will be as close to it as possible. Like this:

// Grab 100 ids under /userPosts/${userId} and return each one as a post model
this.get('store').query('post', {
  path: `userPosts/${userId}`,
  filter: {
    limitToLast: 100
  }
});

// Grab 100 ids under /posts and return each one as a post model
this.get('store').query('post', {
  filter: {
    limitToLast: 100
  }
});

@johra
Copy link

johra commented Feb 20, 2017

Any news on this? Would be VERY nice

@brendanoh
Copy link

People who are watching this issue should take a look at https://github.com/rmmmp/emberfire-utils as it has some cool functionality that could provide some similar benefit.

@mikkopaderes
Copy link
Contributor

For experimentation, I created a branch in emberfire-utils that implements what's being discussed here and some other cool stuffs too.

Check it out here.

@elgordino
Copy link

elgordino commented Jun 5, 2017

I have been thinking about an alternative and maybe orthogonal approach to this problem. My basic requirement is that I can position records belonging to a 'team' under a single path (a team consisting of multiple firebase users). I want the path to be /team/<teamid>/<usual path created by emberfire>

Rather than amending the ID to include a path I have extended the models so they can supply a path back to the adapter when requested. After making this change I was able to relocate all records into the new structure without any change to the rest of my code, all relationships etc worked as before.

I have put my code up in this gist.

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

No branches or pull requests

7 participants