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

MLIBZ-2316: Server Side Delta Set #270

Merged
merged 65 commits into from
May 18, 2018

Conversation

thomasconner
Copy link
Contributor

Description

Add support for server side delta set.

Changes

  • Rename deltaFetch.js to deltaSet.js and move it to under src/core/datastore. This no longer extends the Request class.
  • deltaSet.js add support for sending delta set requests and clearing the query cache.
  • Rename option.useDeltaFetch to option.useDeltaSet on datastore instances. @tejasranade says this does not need to be backwards compatible.

Test

  • Added tests to cachestore.spec.js for sending a delta set request with and without a tagged collection.

@thomasconner thomasconner self-assigned this Mar 14, 2018
nock(client.apiHostname)
.get(`/appdata/${store.client.appKey}/${collection}/_deltaset`)
.query({ since: lastRequestDate.toISOString() })
.reply(200, { changed: [entity2], deleted: [{ _id: entity1._id }] }, {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think entity2 should have an actual change about it. And then the assertion should probably feature a check for that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a change to entity2 and check if that change is made.

import { repositoryProvider } from './repositories';
import { stripTagFromCollectionName, generateEntityId } from './utils';

const QUERY_CACHE_COLLECTION_NAME = '_QueryCache';
Copy link
Contributor

Choose a reason for hiding this comment

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

Constants usually go in the corresponding utils folder.

I feel the name isn't descriptive enough too. It's not a cache and it has more to do with delta set, than with queries (queries are just one part of what the table is used to store). When you read it, you don't think "ah, this is for delta set stuff".

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel differently about the naming. Calling it _QueryCache in the design was a deliberate decision. I am ok with alternatives that describe what it is - it is a log of queries and corresponding timestamps indicating the last time they were run. _QueryHistory, _QueryLog, or equivalent is fine by me.

I want to avoid naming it based on the "delta sync" feature. We could use this table later for other purposes.

Copy link
Contributor

Choose a reason for hiding this comment

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

I hadn't thought of other uses for it. I guess there could be some in the future, I don't know. But "query log" sounds pretty good to me.

I think when the word "cache" is used, it's usually something that has a TTL and what's more important about it is that it is not essential to the work of the system - anything that uses cache, should function without cache, just slower, probably. So I think it would be nice to move away from naming the offline stores "caches" in general. But that's a separate topic :)


const QUERY_CACHE_COLLECTION_NAME = '_QueryCache';

export function deltaSet(collectionNameWithTag, query, options) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this function can be broken down into 5 or so distinct functions, each of which does one thing. I know we've had differences in the past about this, so here's a third-party motivation for this. At the very least, it's worth a read.

Also the name doesn't seem very descriptive. A verb in there would be nice.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 on both comments. I left Victor a very similar comment on the iOS deltaset implementation.
Kinvey/swift-sdk#261 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep this makes sense. I will break up the function into smaller functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I broke up deltaSet() into smaller functions.


export function deltaSet(collectionNameWithTag, query, options) {
const collectionName = stripTagFromCollectionName(collectionNameWithTag);
const serializedQuery = query ? JSON.stringify(query.toQueryString()) : '';
Copy link
Contributor

@gprodanov gprodanov Mar 15, 2018

Choose a reason for hiding this comment

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

Two questions here:

  • query.toString() seems to do precisely this, why not use that
  • the design document doesn't seem to clearly say what's to be kept, but this seems to keep everything about the query. I think there used to be some restriction on this, at least the sort order shouldn't be necessary. reminds me of a comment I had on the first PR - over here. Can we maybe get a confirmation on what should be kept?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An error is now thrown if a query with skip and limit modifiers is used.

let url = format({
protocol: client.apiProtocol,
host: client.apiHost,
pathname: `/appdata/${client.appKey}/${collectionName}`
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be useful here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it was. I am using it now.

.then((data) => {
const { deleted } = data;

if (isArray(deleted) && deleted.length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This check is to check if data is the result of a regular GET or a _deltaset request. I think writing things separately and knowing which request was made above is a better design and improves readability. This ties in with the comment about the size of this function - if it were broken up in smaller functions by responsibility, this wouldn't happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the logic into the correct smaller function.

.then((data) => {
let { changed } = data;

if (!changed) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as the one about line 73.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

@@ -1,316 +0,0 @@
import nock from 'nock';
Copy link
Contributor

Choose a reason for hiding this comment

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

So now we only have one test for delta set? Will there be more added?

Copy link
Contributor Author

@thomasconner thomasconner Mar 16, 2018

Choose a reason for hiding this comment

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

I added 2 more for pull() in cachestore.spec.js. I will probably add a deltaSet.spec.js

@@ -22,8 +23,12 @@ import { buildCollectionUrl } from './utils';

export class NetworkRepository extends Repository {
read(collection, query, options = {}) {
if (options.useDeltaSet) {
return deltaSet(collection, query, options);
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this returns only the modified entities. I don't think this is the spec for delta set in CacheStore.find(). Again, I don't have an app to test it, just judging from the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You were right. It now returns what matches the provided query in cache after the requests are complete.

});
return request.execute()
.then((response) => {
deltaSetQueryDoc.lastRequest = response.headers.get('X-Kinvey-Request-Start');
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a constant for the header name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a const to use for the header name.

Copy link
Contributor

@gprodanov gprodanov left a comment

Choose a reason for hiding this comment

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

I'm not requesting changes as before, because I don't see any bugs.

There is one suggestion, which I would classify as a bug, but it's in the spec doc too, if I understand correctly (the one about skip and take in CacheStore.find()).

There is also one comment about technical debt, which I believe is important to address (the one about redundant updates and deletes), since we are querying in memory.

The rest I view as lower priority, but still important to be noted and fixed. The reason I'm not approving it is we said we'll log tickets for the technical debt and also I don't know if this is going in a beta or not.

import { Client } from '../client';
import { KinveyError } from '../errors';
import { repositoryProvider } from './repositories';
import { buildCollectionUrl } from './repositories/utils';
Copy link
Contributor

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.js is 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.

xKivneyRequestStartHeader
} from './utils';

function getDeltaSetQuery(collectionNameWithTag, query) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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 buildCollectionUrl() function. Not even the entire class.

});
}

function readDocs(collectionNameWithTag, query) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we've used the term "docs" anywhere before - why start now?

Copy link
Contributor

Choose a reason for hiding this comment

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

"entities" is the term we generally use.

return offlineRepo.delete(collectionNameWithTag, deleteQuery)
}

return deleted;
Copy link
Contributor

Choose a reason for hiding this comment

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

offlineRepo.delete() returns the number of deleted entities. deleted can be null/undefined, it seems. Perhaps the method should return 0, to keep return types consistent.

});
}

function updateDocs(collectionNameWithTag, changed = []) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as comments for deleteDocs.


function makeRequest(collectionNameWithTag, deltaSetQuery, query, options) {
if (deltaSetQuery && deltaSetQuery.lastRequest) {
if (query && (query.skip != null || query.limit != null)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

!query.skip seems better than query.skip != null - it's easy to miss that this isn't !== and this is the comparison operator used in js, most commonly. This hinders readability for no apparent reason.

Like the default value for query.skip is 0 and this is why this works, but it wouldn't work with !==, so this is error prone. I mean I agree the "weak" comparison is more readable than checking multiple values, but done with the ! is better than the != operator.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 pull()? "skip" and "limit" should be supported when doing a find().

Copy link
Contributor

Choose a reason for hiding this comment

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

@georgiwe re: your second point - isn't the idea that limit and skip together with a deltaset request is not supported? If I'm reading this code snippet correctly, making a deltaSetQuery with limit and skip is not supported. I think that's reasonable. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

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 .find()s fully functional. In all store types, with all settings. If not, user would need to write their app in a less convenient way and keep track of what works when. If we start throwing an error in CacheStore.find() when skip and limit is passed, and deltaset is enabled, as a developer, it would be difficult to write my app. I may want to make some skip and limit requests - should I disable delta set for those requests only?

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 pull(), but I think it isn't too bad.

Does this make sense, or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This makes sense. I think your suggestion also makes sense. @tejasranade thoughts?

Copy link
Contributor

@tejasranade tejasranade Mar 26, 2018

Choose a reason for hiding this comment

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

Are you suggesting that limit and skip not be applied on the delta request to the backend, but get applied on the subsequent cache request? Doesn't that conflict with the typical purpose of limit and skip (which I imagine is to limit the amount of data being retrieved from the backend)?

How about treating deltaSet and limit/skip as mutually exclusive? If limit / skip is specified, we'll just use it in a standard GET request - regardless of whether the store has deltaset enabled. The assumption is that by specifying limit and skip, the developer has decided what slice of data they're looking for.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

@@ -291,7 +291,6 @@ export class SyncManager {
}

_fetchItemsFromServer(collection, query, options) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this result in redundancies? By "this" I mean

return this._fetchItemsFromServer(collection, query, options)
      .then(entities => this._replaceOfflineEntities(collection, query, entities))
      .then(replacedEntities => replacedEntities.length);

in SyncManager (it's not in the diff). This is the old code and it seems like the updating and deleting (the _replaceOfflineEntities() part) is now being done inside the deltafetch "module". Same goes for when doing a CacheStore.read(). The old DeltaFetchRequest only did a read, the new one does writes too. I guess something else has to change to reflect this too. These redundant deletes and updates would result in worse performance - they are very expensive since we're working in memory.

Copy link
Contributor

@gprodanov gprodanov Mar 19, 2018

Choose a reason for hiding this comment

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

Actually, it's just the updating that's always done. The deleting is done only when doing a delta set request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should something be changed related to this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made a change related to this.

Copy link
Contributor

Choose a reason for hiding this comment

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

No need to duplicate the code - can just add the check in the callback after _fetchItemsFromServer(). And as mentioned above, there is one more place where this happens - in CacheStore.find().

Reading it is kind of odd - "read some and if it's not delta set, replace offline". NetworkRepository does a bit more than expected... But I guess that's the direction we're going.

});
}

function updateDeltaSetQuery(deltaSetQuery, response) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function doesn't seem to need the response. It only needs the header value.

});
}

function makeRequest(collectionNameWithTag, deltaSetQuery, query, options) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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 return). This should be a separate function.

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 makeRequest into all of its small operations and then "merge" makeRequest with deltaSet. The resulting function would delegate all operations to other functions and handle common context. This is readable and each separate function would be cohesive.

After "a request" is made, both "paths" (which should be functions) call updateDeltaSetQuery(). This would be solved by the above paragraph too.


export function deltaSet(collectionNameWithTag, query, options) {
return getDeltaSetQuery(collectionNameWithTag, query)
.then((deltaSetQuery) => makeRequest(collectionNameWithTag, deltaSetQuery, query, options))
Copy link
Contributor

Choose a reason for hiding this comment

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

There was talk about renaming this "query cache" table to "query log" - is this rejected, @tejasranade ? This would make the name of the variable deltaSetQuery (and related functions) something like queryLogEntry, which is more descriptive in my opinion. Calling it "query" right now is incorrect - it isn't a query, a serialized query is part of it. I know you'll say this is all variable names, but it isn't - it's terms which will be used and they should be well defined.

Copy link
Contributor

Choose a reason for hiding this comment

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

The design refers to the additional table as _QueryCache. Since there was some concern about whether it was a "cache", I floated the idea of _QueryLog instead. TBH, I suggest we go with _QueryCache because that's what we're using in other platforms.

In that context, we could use the names - getCachedQuery and cachedQuery here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, _QueryCache it is. Then all that's left is renaming deltaSetQuery to something like "queryCacheEntry", cause cachedQuery sounds like what would be in queryCacheEntry.query. So just one more term to "coin" - what is the entry in the query cache table :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, _QueryCache it is. And I think getCachedQuery and cachedQuery work in this context.

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).

.equalTo('collectionName', collectionNameWithTag)
.and()
.equalTo('query', serializedQuery);
return offlineRepo.read(queryCacheCollectionName, queryCacheQuery)
Copy link
Contributor

@gprodanov gprodanov Mar 19, 2018

Choose a reason for hiding this comment

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

I now remember we went a different direction with the kinvey_sync collection - it is being tagged like any other collection. Here with the "query cache" collection, we are only scoping the collection name "column" of its "rows". Should we may be unify our approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand what you mean. Can you explain more?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean that when you have a data store with no tag, it uses a collection kinvey_sync for the sync queue. If you use a data store with a tag, for the same collection, it uses a collection called kinvey_sync.<tag-name> for the sync queue.

The same two cases, regarding the _QueryCache collection, would use the same collection. Just the values for the collection "column" would be different. The same could have been applied to the kinvey_sync collection, but we went a different route. I'm saying we should decide which is better, and a unified behaviour might be a consideration in this decision.

});
}

function deleteDocs(collectionNameWithTag, deleted = []) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This method signature is confusing. Is the purpose of the method to delete the entities that are passed in as an array parameter? If so, it's clearer to name that variable something like entitiesToDelete rather than deleted.

function makeDeltaSetRequest(collectionNameWithTag, deltaSetQuery, query, options) {
const collectionName = stripTagFromCollectionName(collectionNameWithTag);
const client = Client.sharedInstance();
const request = new KinveyRequest({
Copy link
Contributor

Choose a reason for hiding this comment

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

@georgiwe I don't completely understand your comment. What KinveyRequest.execute() call are you referring to? Is there an example.

I agree in principle with your point, though. It seems like a bunch of these options (skipBL, timeout etc) would apply to all data requests, so some way to avoid listing them out one-by-one will be useful.


function makeRequest(collectionNameWithTag, deltaSetQuery, query, options) {
if (deltaSetQuery && deltaSetQuery.lastRequest) {
if (query && (query.skip != null || query.limit != null)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@georgiwe re: your second point - isn't the idea that limit and skip together with a deltaset request is not supported? If I'm reading this code snippet correctly, making a deltaSetQuery with limit and skip is not supported. I think that's reasonable. Thoughts?

function makeRequest(collectionNameWithTag, deltaSetQuery, query, options) {
if (deltaSetQuery && deltaSetQuery.lastRequest) {
if (query && (query.skip != null || query.limit != null)) {
throw new KinveyError('You cannot use the skip and limit modifiers on the query when performing a delta set request.');
Copy link
Contributor

Choose a reason for hiding this comment

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

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 throw (err) seems ok. WDYT?

throw new KinveyError('You cannot use the skip and limit modifiers on the query when performing a delta set request.');
}

return makeDeltaSetRequest(collectionNameWithTag, deltaSetQuery, query, options);
Copy link
Contributor

Choose a reason for hiding this comment

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

@georgiwe made a point about the makeRequest naming earlier in the review, and I find it relevant here. If the same class has makeRequest and makeDeltaSetRequest functions, it's not clear what each is doing.


export function deltaSet(collectionNameWithTag, query, options) {
return getDeltaSetQuery(collectionNameWithTag, query)
.then((deltaSetQuery) => makeRequest(collectionNameWithTag, deltaSetQuery, query, options))
Copy link
Contributor

Choose a reason for hiding this comment

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

The design refers to the additional table as _QueryCache. Since there was some concern about whether it was a "cache", I floated the idea of _QueryLog instead. TBH, I suggest we go with _QueryCache because that's what we're using in other platforms.

In that context, we could use the names - getCachedQuery and cachedQuery here.


return promise
.then((response) => {
const requestStartDate = response.headers.get(xKivneyRequestStartHeader);
Copy link
Contributor

Choose a reason for hiding this comment

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

Just noticed a typo here while randomly browsing code - xKivneyRequestStartHeader :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we don't need the "x" in there too, while we're on the topic :)

Thomas Conner added 8 commits March 22, 2018 11:09
* master:
  Minor fixes for variable names, remove outdated comments, improve error messages.
  MLIBZ-2133 Use es6-promise promises in live-service-related files, instead of native ones.
  minor test and error handling improvements
  Use long form clientID in MIC endpoints

# Conflicts:
#	src/core/datastore/sync/sync-manager.js
@thomasconner
Copy link
Contributor Author

I think all requested changes have been made. Is there anything else missing that is preventing this PR from being approved?

@thomasconner
Copy link
Contributor Author

@tejas I fixed the skipped unit tests.

* MLIBZ-2478:
  MLIBZ-2478: Return count of items changed when making a _deltaset request with pull()
* MLIBZ-2477:
  MLIBZ-2477: Handle NotFoundError when deleting a cachedQuery
* MLIBZ-2486:
  MLIBZ-2486: Ignore limit and skip for auto pagination and update tests
* MLIBZ-2493:
  MLIBZ-2493: Perform delta set delete and then delta set changed serially instead of in parallel.
* MLIBZ-2449:
  MLIBZ-2449: Do not filter out _QueryCache table or __testSupport__ table when get all tables for WebSQL
* MLIBZ-2495:
  MLIBZ-2495: Ignore deleting entities when limit and skip are used to pull entities
@thomasconner
Copy link
Contributor Author

Can I have this PR looked over again? There have been several bug fixes made over the past few days related to testing.

@tejasranade
Copy link
Contributor

@thomasconner this branch needs a merge from master. Once you do that, we can do a final review before merging.

@thomasconner
Copy link
Contributor Author

Master has been merged.

thomasconner and others added 6 commits May 18, 2018 09:31
* MLIBZ-2512:
  MLIBZ-2512: Ignore skip value of query for auto pagination
* Added deltaset tests

* added new tests for limit and skip and some unit tests

* skipped tests that use websql to not run on other than html5 and removed a .only

* removed another .only

* did some refactoring and fixed tests failing for Nativescript and Phonegap

* Fix status code set with nock for failing unit test

* Added deltaset tests

* added new tests for limit and skip and some unit tests

* skipped tests that use websql to not run on other than html5 and removed a .only

* removed another .only

* did some refactoring and fixed tests failing for Nativescript and Phonegap

* Fix status code set with nock for failing unit test

* added a test for autopagination with skip and limit

* fixing an expectation string and wrong expectation for deltaset test

* Added deltaset tests

* added new tests for limit and skip and some unit tests

* skipped tests that use websql to not run on other than html5 and removed a .only

* removed another .only

* did some refactoring and fixed tests failing for Nativescript and Phonegap

* Fix status code set with nock for failing unit test

* removed .only again
@thomasconner thomasconner merged commit 692bf67 into master May 18, 2018
@thomasconner thomasconner deleted the MLIBZ-2316_server_side_delta_set branch May 18, 2018 18:51
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.

3 participants