MLIBZ-2316: Server Side Delta Set #270
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,159 @@ | ||
| import isArray from 'lodash/isArray'; | ||
| import { format } from 'url'; | ||
| import { Query } from '../query'; | ||
| import { KinveyRequest, RequestMethod, AuthType } from '../request'; | ||
| import { Client } from '../client'; | ||
| import { KinveyError } from '../errors'; | ||
| import { repositoryProvider } from './repositories'; | ||
| import { buildCollectionUrl } from './repositories/utils'; | ||
| import { | ||
| stripTagFromCollectionName, | ||
| generateEntityId, | ||
| queryCacheCollectionName, | ||
| xKivneyRequestStartHeader | ||
| } from './utils'; | ||
|
|
||
| function getDeltaSetQuery(collectionNameWithTag, query) { | ||
|
There was a problem hiding this comment. Don't need the "withTag" part of the variable name. We're trying to not think about that. The only place which knows about the tag is the |
||
| const serializedQuery = query ? query.toString() : ''; | ||
|
|
||
| return repositoryProvider.getOfflineRepository() | ||
| .then((offlineRepo) => { | ||
| const queryCacheQuery = new Query() | ||
| .equalTo('collectionName', collectionNameWithTag) | ||
| .and() | ||
| .equalTo('query', serializedQuery); | ||
| return offlineRepo.read(queryCacheCollectionName, queryCacheQuery) | ||
|
There was a problem hiding this comment. I now remember we went a different direction with the There was a problem hiding this comment. I don't understand what you mean. Can you explain more? There was a problem hiding this comment. I mean that when you have a data store with no tag, it uses a collection The same two cases, regarding the |
||
| .then((deltaSetQueryDocs = []) => { | ||
| if (deltaSetQueryDocs.length > 0) { | ||
| return deltaSetQueryDocs[0]; | ||
| } | ||
|
|
||
| return { | ||
| _id: generateEntityId(), | ||
| collectionName: collectionNameWithTag, | ||
| query: serializedQuery | ||
| }; | ||
| }); | ||
| }); | ||
| } | ||
|
|
||
| function updateDeltaSetQuery(deltaSetQuery, response) { | ||
|
There was a problem hiding this comment. This function doesn't seem to need the response. It only needs the header value. |
||
| return repositoryProvider.getOfflineRepository() | ||
| .then((offlineRepo) => { | ||
| deltaSetQuery.lastRequest = response.headers.get(xKivneyRequestStartHeader); | ||
| return offlineRepo.update(queryCacheCollectionName, deltaSetQuery); | ||
| }); | ||
| } | ||
|
|
||
| function readDocs(collectionNameWithTag, query) { | ||
|
There was a problem hiding this comment. I don't think we've used the term "docs" anywhere before - why start now? There was a problem hiding this comment. "entities" is the term we generally use. |
||
| return repositoryProvider.getOfflineRepository() | ||
| .then((offlineRepo) => { | ||
| return offlineRepo.read(collectionNameWithTag, query); | ||
| }); | ||
| } | ||
|
|
||
| function deleteDocs(collectionNameWithTag, deleted = []) { | ||
|
There was a problem hiding this comment. This method signature is confusing. Is the purpose of the method to |
||
| return repositoryProvider.getOfflineRepository() | ||
| .then((offlineRepo) => { | ||
| if (isArray(deleted) && deleted.length > 0) { | ||
| const deletedIds = deleted.map((doc) => doc._id); | ||
| const deleteQuery = new Query().contains('_id', deletedIds); | ||
| return offlineRepo.delete(collectionNameWithTag, deleteQuery) | ||
| } | ||
|
|
||
| return deleted; | ||
|
There was a problem hiding this comment.
|
||
| }); | ||
| } | ||
|
|
||
| function updateDocs(collectionNameWithTag, changed = []) { | ||
|
There was a problem hiding this comment. Same as comments for |
||
| return repositoryProvider.getOfflineRepository() | ||
| .then((offlineRepo) => { | ||
| if (isArray(changed) && changed.length > 0) { | ||
| return offlineRepo.update(collectionNameWithTag, changed) | ||
| } | ||
|
|
||
| return changed; | ||
| }); | ||
| } | ||
|
|
||
| function makeDeltaSetRequest(collectionNameWithTag, deltaSetQuery, query, options) { | ||
| const collectionName = stripTagFromCollectionName(collectionNameWithTag); | ||
|
There was a problem hiding this comment. Like we said, only |
||
| const client = Client.sharedInstance(); | ||
| const request = new KinveyRequest({ | ||
|
There was a problem hiding this comment. Why not use There was a problem hiding this comment. @georgiwe I don't completely understand your comment. What I agree in principle with your point, though. It seems like a bunch of these options ( There was a problem hiding this comment. I mean like here. It basically takes care of all these defaults which are used over 90% of the time, extracting the data, etc. Just getting rid of boilerplate. But perhaps you meant something else. I was saying ignore things like skipBL, since they are not documented (reference). We can expand on the utilities around making a request, but about a quarter of the old data store classes were code which made requests, many of these options either not documented/supported or rarely used, and got copied around the codebase. |
||
| authType: AuthType.Default, | ||
| method: RequestMethod.GET, | ||
| url: format({ | ||
| protocol: client.apiProtocol, | ||
| host: client.apiHost, | ||
| pathname: buildCollectionUrl(collectionName, '_deltaset'), | ||
|
There was a problem hiding this comment.
|
||
| query: { since: deltaSetQuery.lastRequest } | ||
| }), | ||
| query, | ||
| timeout: options.timeout, | ||
| followRedirect: options.followRedirect, | ||
| cache: options.cache, | ||
| properties: options.properties, | ||
| skipBL: options.skipBL, | ||
| trace: options.trace, | ||
| client | ||
| }); | ||
|
|
||
| return request.execute() | ||
| .then((response) => { | ||
| return updateDeltaSetQuery(deltaSetQuery, response).then(() => response.data); | ||
| }) | ||
| .then((data) => { | ||
| return deleteDocs(collectionNameWithTag, data.deleted) | ||
| .then(() => updateDocs(collectionNameWithTag, data.changed)) | ||
| .then(() => data); | ||
| }); | ||
| } | ||
|
|
||
| function makeRequest(collectionNameWithTag, deltaSetQuery, query, options) { | ||
|
There was a problem hiding this comment. This function can very easily be split into two - it does two (and a half) things - performs a delta set request, or a regular get request (and the "half" is managing how local store should be updated). It's clearly visible where one, self-sufficient part ends (the first The name "makeRequest" even suggests things should be different - if you can't think of a descriptive name for a function, it means it's not clear what it does. Which means it should change. I think the way to solve this is split After "a request" is made, both "paths" (which should be functions) call |
||
| if (deltaSetQuery && deltaSetQuery.lastRequest) { | ||
| if (query && (query.skip != null || query.limit != null)) { | ||
|
There was a problem hiding this comment.
Like the default value for There was a problem hiding this comment. Another point is that this check would return the error even when doing a CacheStore.find(). I asked this question in the design doc too, but shouldn't this be only for There was a problem hiding this comment. @georgiwe re: your second point - isn't the idea that There was a problem hiding this comment. Sure, and I didn't think of this at the time, but yesterday I kinda changed my mind. And this may affect the way we treat skip and limit, if we want to keep some consistency. It seems important to keep One way to keep them, is not send them as part of the delta set request (ignore them), but apply them to the request to the offline store, here. This way we "update the local store", and then read whatever the user is trying to read, from a "synced" place. This approach may introduce some confusion as it's different from Does this make sense, or am I missing something? There was a problem hiding this comment. This makes sense. I think your suggestion also makes sense. @tejasranade thoughts? There was a problem hiding this comment. Are you suggesting that How about treating deltaSet and There was a problem hiding this comment. Yes, this is what I'm suggesting. I don't think it conflicts. I think skip and limit help you target a specific subset of data (just like you said), usually when paging things, but not always. Not just from the server, but from any data source. Cause if you had to pull everything, just to take the last 3 entities, things don't make sense, regardless of the data source. I guess turning off deltaset would make the API easier to work with, but I think what I suggested is better - we get the best of both worlds. We get performance, we get the expected result and an easy to use API. What do you dislike about it - you feel it's semantically wrong? Ultimately, I'm fine with whatever you decide - the most important part is to have an easy to use API, which works correctly and as expected. |
||
| throw new KinveyError('You cannot use the skip and limit modifiers on the query when performing a delta set request.'); | ||
|
There was a problem hiding this comment.
There was a problem hiding this comment. Aren't they equivalent, in how we're using them? I wasn't quite sure of the difference, so I looked up this post - https://stackoverflow.com/questions/33445415/javascript-promises-reject-vs-throw Now I think our use of There was a problem hiding this comment. If an exception is thrown somewhere in a promise chain, it's essentially the same. Like so: function someFunc() {
throw new Error('aaa');
}
Promise.resolve()
.then(() => {
someFunc();
})
.catch(err => {
console.log('caught in promise.catch()', err.message); // the thrown error
});But, if this function is called outside of a promise chain like so: try {
someFunc() // throws a synchronous exception
.catch(err => {
console.log('err', err.message); // doesn't fire
});
} catch (err) {
console.log('caught in catch', err.message);
}Currently, this particular function is only called in a promise, but it's not robust - if moved outside, things might break unexpectedly. Another side of the reason is that a promise represents "a value in time". And the reason thrown exceptions are "caught" by promises is precisely this - they shouldn't be synchronous. There was a problem hiding this comment. Oh! Totally forgot about the link you sent, sorry. Yeah, that use case is even trickier - throwing on the next processor tick. It's just a matter of not expecting a synchronous exception, when working with promises. Reading the function doesn't necessarily tell you "I should only use this in promise chains". We should do what essentially the example in the answer you linked does - wrap the exception in a promise. |
||
| } | ||
|
|
||
| return makeDeltaSetRequest(collectionNameWithTag, deltaSetQuery, query, options); | ||
|
There was a problem hiding this comment. @georgiwe made a point about the |
||
| } | ||
|
|
||
| const collectionName = stripTagFromCollectionName(collectionNameWithTag); | ||
| const client = Client.sharedInstance(); | ||
| const request = new KinveyRequest({ | ||
| authType: AuthType.Default, | ||
| method: RequestMethod.GET, | ||
| url: format({ | ||
| protocol: client.apiProtocol, | ||
| host: client.apiHost, | ||
| pathname: buildCollectionUrl(collectionName) | ||
| }), | ||
| query, | ||
| timeout: options.timeout, | ||
| followRedirect: options.followRedirect, | ||
| cache: options.cache, | ||
| properties: options.properties, | ||
| skipBL: options.skipBL, | ||
| trace: options.trace, | ||
| client | ||
| }); | ||
| return request.execute() | ||
| .then((response) => { | ||
| return updateDeltaSetQuery(deltaSetQuery, response).then(() => response.data); | ||
| }) | ||
| .then((data) => { | ||
| return updateDocs(collectionNameWithTag, data); | ||
| }); | ||
| } | ||
|
|
||
| export function deltaSet(collectionNameWithTag, query, options) { | ||
| return getDeltaSetQuery(collectionNameWithTag, query) | ||
| .then((deltaSetQuery) => makeRequest(collectionNameWithTag, deltaSetQuery, query, options)) | ||
|
There was a problem hiding this comment. There was talk about renaming this "query cache" table to "query log" - is this rejected, @tejasranade ? This would make the name of the variable There was a problem hiding this comment. The design refers to the additional table as In that context, we could use the names - There was a problem hiding this comment. Alright, There was a problem hiding this comment. Okay, Just one more term to define - what is one entry in that table called? Cause it shouldn't be a "query" (and that's kinda why I didn't want the table to be a cache for queries). |
||
| .then(() => readDocs(collectionNameWithTag, query)) | ||
| } | ||
|
|
||
| export function clearDeltaSet(collectionNameWithTag) { | ||
| const queryCacheQuery = new Query().equalTo('collectionName', collectionNameWithTag); | ||
| return repositoryProvider.getOfflineRepository() | ||
| .then((offlineRepo) => offlineRepo.delete(queryCacheCollectionName, queryCacheQuery)); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason utils are not reexported from
/repositories/index.jsis that they were local, used only there. Once something is being used elsewhere, it should move to another utils folder, some levels up, at the lowest level, where it can be shared accordingly.