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

Add support for db.import #265

Merged
merged 1 commit into from
Nov 25, 2015
Merged

Add support for db.import #265

merged 1 commit into from
Nov 25, 2015

Conversation

Natim
Copy link
Member

@Natim Natim commented Nov 20, 2015

For some of the Mozilla platform use-cases, it's desirable to have the ability to import initial data. Since we only need it here, for the time being, we can prototype the idea in the Firefox storage adapter and apply what we learn to the other adapters.

@n1k0
Copy link
Contributor

n1k0 commented Nov 18, 2015

As discussed previously and as a first easy step before we implement full support for transactions, the base adapter interface should require implementation of an import(<array>) method, to import a large list of records using a single transaction, mostly for performance reasons.

This method should return a Promise, with a simple result like the number of imported records. It should reject when the transaction fails, probably simply by forwarding the backend error.

This method should probably be exposed at the Collection level as well, so users can bulk import large datasets right from their collection instance; sample api:

const coll = kinto.collection("articles");
const recordsToImport = [{title: "art00001", ..., }, ..., {title: "art99999", ...}];
coll.import(recordsToImport)
  .then(res => {
    console.log(`${res} records were imported.`);
  });

@n1k0 n1k0 added this to the 1.2 milestone Nov 18, 2015
@Natim Natim self-assigned this Nov 20, 2015
@Natim
Copy link
Member

Natim commented Nov 20, 2015

We've added adapter import method for LocalStorage and IndexedDB with @leplatrem.

@leplatrem
Copy link
Contributor

Remaining tasks:

  • Documentation
  • Move of updated last modified to adapter tests
  • Update collection last modified only if superior

@Natim
Copy link
Member

Natim commented Nov 20, 2015

In order to update the FirefoxStorage adapter, we need to add tests in Firefox.

Here is the flow to run the FirefoxStorage tests:

  1. kinto.js> npm run dist-fx
  2. cp ~/kinto.js/dist/moz-kinto-client.js ~/moz-central/services/common/
  3. moz-central> ./mach build
  4. Update services/common/tests/unit/test_storage_adapter.js and services/common/tests/unit/test_kinto.js
  5. Run the tests with ./mach xpcshell-test services/common/tests/unit/test_storage_adapter.js

@Natim
Copy link
Member

Natim commented Nov 23, 2015

Ok so we are almost there I guess.

The remaining problem to handle is the collection_timestamp handling.

We do not want import to overwrite records in a more recent version.
So we need to make sure that the collection_timestamp we are importing is either null or lower than the minimal last_modified value for the records we are importing.


if (!records.every(record => {
return record.id && this.idSchema.validate(record.id);
})) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How about rejecting at first invalid record encountered instead? If we provide the id, that's useful for debugging.

Copy link
Contributor

Choose a reason for hiding this comment

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

with some() ?

@Natim
Copy link
Member

Natim commented Nov 23, 2015

An even better solution would be to import only records of the dump with a last_modified value newer than the current collection_timestamp.

}

if (!records.every(record => record.last_modified)) {
return reject("Record has no last_modified value.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

@mozmark
Copy link
Contributor Author

mozmark commented Nov 23, 2015

The difficulty with only importing items with lastModified later than the newest value is that you will end up keeping things that should be removed. I'd suggest either 1) Only allow import of a new collection or 2) Allow a collection to be completely replaced if (and only if) the imported data has a last modified value newer than the old collection.

@Natim
Copy link
Member

Natim commented Nov 23, 2015

Yes that's the idea.

Also you raise an important point, we should probably handle tombstones in our import in order to remove deleted records.

@Natim
Copy link
Member

Natim commented Nov 24, 2015

  • Rename import to loadDump
  • Skip records that were already updated (local record status: synced and last_modified value more recent than the one in the dump or status: updated).
  • Result of promise should return the list of modified records.

@mozmark
Copy link
Contributor Author

mozmark commented Nov 25, 2015

This looks good to me. I guess we r? @n1k0 when ready.

@@ -214,6 +214,29 @@ articles.list({sort: "-title"})
> - By default, the records are sorted on `last_modified` in descending order.
> - As mentioned in the [limitations](limitations.md) section, the sort is performed in memory.

## Importing initial records locally
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Importing a data dump locally

@n1k0
Copy link
Contributor

n1k0 commented Nov 25, 2015

LGTM, r+

leplatrem added a commit that referenced this pull request Nov 25, 2015
Add Collection#loadDump() method (fixes #265)
@leplatrem leplatrem merged commit fca5862 into master Nov 25, 2015
@leplatrem leplatrem deleted the 265-import-records-feature branch November 25, 2015 12:00

> #### Notes
>
> - Existing records are replaced if they do not have more recent modifications.
Copy link

Choose a reason for hiding this comment

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

This…

leplatrem added a commit that referenced this pull request Dec 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants