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

filter results of pouch adapter query by correct type #194

Merged
merged 1 commit into from
Sep 12, 2017
Merged

filter results of pouch adapter query by correct type #194

merged 1 commit into from
Sep 12, 2017

Conversation

akatov
Copy link
Contributor

@akatov akatov commented Sep 8, 2017

right now when using query, the ember-pouch adapter actually returns all document types from the pouchdb that match the selector...

Copy link
Collaborator

@jlami jlami left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this solution, but another way to go might be to add extra parameters for the find selector. This will give some extra requirements on the needed index, which might be hard to get right. But this would put the filtering on the pouch/couch side, which could be worth something.
But this simpler solution could be good to try first?

@@ -351,7 +351,15 @@ export default DS.RESTAdapter.extend({
queryParams.sort = this._buildSort(query.sort);
}

return db.find(queryParams).then(pouchRes => db.rel.parseRelDocs(recordTypeName, pouchRes.docs));
var idType = type.documentType || recordTypeName;
var recordTypeNameRegex = new RegExp('^' + idType + '_[0-3]');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if a regex is really the right way to go here. Feels a bit overkill. But using the normal way to parse the ID with parseDocID might also be too much. A id.startsWith(idType + '') might be enough here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, maybe babel does not have polyfills yet for startsWith by default? So the RegExp should probably be ok. Technically the idType should be escaped, but the characters that would need escaping would probably not be correct Ember Model names

@broerse
Copy link
Collaborator

broerse commented Sep 12, 2017

@akatov I agree with @jlami that it is better to pass an extra parameter to the find selector. But for now this PR does the job. Thanks!

@broerse broerse merged commit ffb79cc into pouchdb-community:master Sep 12, 2017
@afinne
Copy link

afinne commented Sep 13, 2017

A bit late now, but a comment nonetheless. This solution makes essentially skip and limit unreliable. You might end up with no results, even though data exists.

A solution more along what has been suggested in #168 makes more sense.

What we ended up implementing in our project is to actually have a _typeFoo: true property for model Foo and add that in as a criteria automatically when querying.
The reason we did not go for the _id approach was that we got problems with index selection. I.e. the find plugin decided to use our custom index instead of the special _all_docs index when trying to follow an ember one-to-one ('belongsTo') relationship. This issue might have been fixed in the latest pouchdb-find. I haven't tried out the _id approach with that one.

@broerse
Copy link
Collaborator

broerse commented Sep 13, 2017

@afinne It is not released yet. Your point is valid. @akatov should we revert this PR?

@akatov
Copy link
Contributor Author

akatov commented Sep 13, 2017

@broerse probably best to revert and solve this properly. Also, I hadn't seen #168...

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

Successfully merging this pull request may close these issues.

4 participants