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

Make bulkUpdate non atomic function #3433

Closed
wants to merge 1 commit into from
Closed

Conversation

ssh24
Copy link
Contributor

@ssh24 ssh24 commented Jun 6, 2017

Description

Make bulkUpdate a non atomic function. Allow connectors to perform bulkUpdates now.

connect to loopbackio/loopback-connector-cloudant#35

@ssh24 ssh24 force-pushed the add-bulkUpdate-function branch from d2bdd5d to 205559b Compare June 6, 2017 00:17
@bajtos
Copy link
Member

bajtos commented Jun 6, 2017

@ssh24 Could you please explain what problem is this pull request trying to solve?

bulkUpdate API is an integral part of change replication/offline sync implementation. I am concerned that by moving this implementation down to connectors, we make it easy for 3rd-party/community connectors to break change replication by implementing an incomplete version of bulkUpdate. To address that, I would like to see a robust test suite covering bulkUpdate in juggler's shared test suite run by all connectors.

I also think this is a breaking (semver-major) change - existing applications will stop working after they update to a newer loopback version (with bulkUpdate removed) in case their connector does not provide its own bulkUpdate implementation. A possible solution for preserving backwards-compatibility is to keep bulkUpdate implementation inside loopback, mark it as deprecated, and let connectors to override this loopback-provided version with their own implementation, similarly how they are already overriding other DAO methods.

Copy link
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

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

☝️

Allow connectors to perform bulkUpdates now.

This should be already possible, using the same mechanism which overrides dummy "create" with "create" provided by DataAccessObject.

@ssh24
Copy link
Contributor Author

ssh24 commented Jun 6, 2017

@bajtos This PR and the subset PR's are not complete. Thank you for joining the conversation. I would like @raymondfeng and @kjdelisle to join as well.

PR in juggler: loopbackio/loopback-datasource-juggler#1399
PR in cloudant: loopbackio/loopback-connector-cloudant#150

The goal here was to allow cloudant connector perform bulkUpdate. This is definitely a breaking change in loopback. The tests are failing as well on bulkUpdate. So I want opinion on this and what would be the best way to do it.

@ssh24
Copy link
Contributor Author

ssh24 commented Jun 6, 2017

I realized we can override this function in DAO. Closing this PR.

@ssh24 ssh24 closed this Jun 6, 2017
@ssh24 ssh24 deleted the add-bulkUpdate-function branch June 6, 2017 14:19
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.

2 participants