Part 1 of moving tx timeout and id to connector

[lehni]

Description

This simple PR is part one of moving the handling of transaction timeouts and ids from loopback-datasource-juggler's TransactionMixin.beginTransaction() to loopback-connector's Transaction.begin(), where it makes more sense and can be leveraged more easily by other parts of the code, e.g. the work I am doing in #1472. For a more detailed description of the motivation to do so, see #1472 (comment)

By itself, this does not change anything yet, it just adds support for moving / (duplicating for the time being) the functionality in loopback-connector, without calling the timeout observers twice.

As the comment in this PR suggests, we should keep this code around for quite a while and eventually remove it:

// NOTE(lehni) As part of the process of moving the handling of
// transaction id and timeout from TransactionMixin.beginTransaction() to
// Transaction.begin() in loopback-connector, switch to only setting id
// and timeout if it wasn't taken care of already by Transaction.begin().
// Eventually, we can remove the following two if-blocks altogether.

I don't think additional tests are required.

Related issues

• Part 2 of moving tx timeout and id to connector loopback-connector#116
• Add a better way to handle transactions by binding them to models #1472

Checklist

• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide

[lehni]

Regarding coverage/coveralls, I don't understand how this change can decrease the coverage?

[kjdelisle]

@lehni Technically, the extension of the if statement to incorporate a check against the transaction's timeout property constitutes a "new code path" of sorts, which is what coveralls is probably complaining about. That being said, since this isn't a breaking change, and is meant as a stop-gap to avoid breakage with subsequent PRs, I'm okay with the small decrease.

[kjdelisle]

One additional note: Coveralls seems to experience rounding errors with sufficiently large code bases; I've had it report coverage decreases, only to re-run the tests again and have it report that there was no change in coverage. (I'm not certain that this is the case, but I've seen it happen and couldn't come up with an alternative explanation.)

[kjdelisle]

@lehni Please reword the commit message to be about the change as it stands; something like "transaction: bind timeout to tx instance" and then I'll land this.

[lehni]

@kjdelisle great! I've adjusted this now. Thanks!

[lehni]

After further looking into transactions in loopback-datasource-juggler and loopback-connector, I believe I didn't go far enough with this PR:

There is a whole lot of monkey-patching going on in loopback-datasource-juggler/lib/transaction.js, and I believe it should all go to loopback-connector/blob/master/lib/transaction.js for clarity instead. The changes are basically about promisifying things, but not exclusively so:

https://github.com/strongloop/loopback-datasource-juggler/blob/f18d3487c64ea1ed3896da4d7a5010eadbd59a0d/lib/transaction.js#L115-L219

VS:

https://github.com/strongloop/loopback-connector/blob/f2cbdedee5800c33a977be8d58f5c8b1a30143d3/lib/transaction.js#L48-L59

I would like to suggest to create another pair of PRs that moves it all to loopback-connector and cleans things up there. I would create the PRs in a way that both would have to be released at the same time and would require each other as minimum dependencies, without any backward compatibility. I think this is fine even if related loopback-connector-* packages are using older versions of loopback-connector, due to these lines here:

https://github.com/strongloop/loopback-connector/blob/f2cbdedee5800c33a977be8d58f5c8b1a30143d3/lib/transaction.js#L94-L97

Before I embark on this, I'd like to hear thoughts on the proposal... @kjdelisle, @bajtos, @ssh24 does this sound reasonable?

[kjdelisle]

I think that sounds good. You've already thought of the idea of enforcing the mutual dependency to protect the API integrity for end-users. We'll find out regardless once the downstream dependencies run in CI.

[lehni]

After looking more into it, I realised that things are a bit more complicated unfortunately:

As part of the monkey-patching, the ObserverMixin is mixed into Transaction in https://github.com/strongloop/loopback-datasource-juggler/blob/f18d3487c64ea1ed3896da4d7a5010eadbd59a0d/lib/transaction.js#L117

And ObserverMixin is part of loopback-datasource-juggler.

So what we could do is move everything else over to loopback-connector, but keep the part that promisifies Transaction#commit() and Transaction#rollback(), and render them observable at the same time... Or we could have loopback-connector import ObserverMixinfrom loopback-datasource-juggler.

I am not sure what's better?

[kjdelisle]

The first option would be better, mostly because the juggler is not a dependency of the connector base (it's only a dev dep for testing purposes).

[bajtos]

I have mixed feelings about moving ObserverMixin to loopback-connector. Conceptually, juggler used to depend on loopback-connector only because we were including few built-in connectors like memory and kv-memory. Ideally, only these connector implementations should depend on loopback-connector package.

ObserverMixin is used by DataAccessObject. If we move ObserverMixin to loopback-connector, we will start going down the path where loopback-connector is becoming an integral part/dependency of juggler and will be difficult to decouple/remove in the future. OTOH, we already made few steps in this direction because we are gettingTransaction implementation from loopback-connector.

Setting aside design purity, there are also practical implications. loopback-connector package was created as part of the redesign to get rid connectors having a peer dependency on juggler, which was needed at that time because the shared connector code was living in juggler. See strongloop/loopback#275 for more details. Now the idea behind loopback-connector is that this package contains shared helpers/base classes used by connectors, and it allows connectors to be version-independent from juggler and vice versa.

For example, consider the following dependency tree (using the latest versions as of this writing):

MyProject
 - loopback-datasource-juggler@3.x
    - loopback-connector@4.x        
 - loopback-connector-mysql@5.x
    - loopback-connector@4.x        
 -loopback-connector-sqlite@1.x
    - loopback-connector@2.x

In the original design, the sqlite datasource and attached models will be using the latest code for things that are not connector specific (like ObserverMixin) and possibly an older code for things sqlite-specific (because sqlite was not upgraded yet).

Now my concern with moving generic stuff from juggler to connector is that once done, we can end up in a situation where the connector relying on an older version of loopback-connector will start seeing newer functionality from the loopback-connector version installed as a dependency of juggler, and the connector may not be prepared to handle that.

In an ideal world, I think the solution to these problems would be to decouple Transaction/ObserverMixin from juggler even more and ship them as standalone packages (loopback-transactions, loopback-observer). OTOH, I am not sure how big the issue described above really is, and whether the overhead of maintaining two more packages is worth the gains.

Also, maybe DataAccessObject does not really belong to juggler and should be part of loopback-connector base? I am thinking along the lines that DataAccessObject is used only by a certain kind of connectors (implementing CRUD semantics), but there are many other connectors that don't use it at all (REST, SOAP) or use a different data-access object (KeyValueAccessObject). In which case it would make sense to move both ObserverMixin and *AccessObject into loopback-connector, together with the (shared) test cases, but that's a huge change that we probably don't want to make right now, especially considering the long-term plans for juggler refactoring.

Uh oh.

I am not sure what's the best next step here. I guess we can either move ObserverMixin to loopback-connector and live with the consequences, or move ObserverMixin to a new package and make it a common dependency of both loopback-datasource-juggler and loopback-connector.

Or we could have loopback-connector import ObserverMixinfrom loopback-datasource-juggler.

I think this is definitely not the option, one of the reasons is that we will have circular dependency (juggler's memory connector depends on loopback-connector which depends on juggler).

Another alternative comes to my mind: can we perhaps inject ObserverMixin to Transaction, e.g. via a constructor argument?

[lehni]

Another alternative comes to my mind: can we perhaps inject ObserverMixin to Transaction, e.g. via a constructor argument?

Not really, because the Transaction object gets created in Transaction.begin() which is part of loopback-connector, not loopback-datasource-juggler. We would have to pass the mixin to the begin function, and that would be strange I think. And Transaction would then always have to see if this.notifyObserversAround was actually there before calling it in commit() and rollback(), not ideal either.

Right now, ObserverMixin is used in DataSource, ModelBaseClass and Transaction of loopback-datasource-juggler.

I agree that creating a new loopback-observer may be the way to go. I don't think we need to take Transaction out of loopback-connector though, where most of its parts resides currently and where I think it belongs, we just need to take the monkey-patching out of loopback-datasource-juggler and move it there too.

Does that make sense?

Another options could be to start a loopback-core package that contains such abstract core classes, mixins and methods. This could be a good place also to put the utils.createPromiseCallback() that currently is replicated all across the loopback codebase (another thorn in my eye...).

[bajtos]

@lehni

Another alternative comes to my mind: can we perhaps inject ObserverMixin to Transaction, e.g. via a constructor argument?

Not really, because the Transaction object gets created in Transaction.begin() which is part of loopback-connector, not loopback-datasource-juggler. We would have to pass the mixin to the begin function, and that would be strange I think. And Transaction would then always have to see if this.notifyObserversAround was actually there before calling it in commit() and rollback(), not ideal either.

Makes sense, thank you for explaining it!

I agree that creating a new loopback-observer may be the way to go. I don't think we need to take Transaction out of loopback-connector though, where most of its parts resides currently and where I think it belongs, we just need to take the monkey-patching out of loopback-datasource-juggler and move it there too.

I am not familiar enough with juggler to fully comprehend what do you mean by monkey-patching Transactions, but hearing the phrase monkey-patching makes me want to do what you propose :)

Another options could be to start a loopback-core package that contains such abstract core classes, mixins and methods. This could be a good place also to put the utils.createPromiseCallback() that currently is replicated all across the loopback codebase (another thorn in my eye...).

Perhaps. The downside of such module is that over time, it may become a kitchen sink for utility functions, and then we will have hard times making breaking changes, because everybody will be depending on this module. We went through a similar process with loopback package - we put auth, offline-sync, email, etc. there, because it was the easiest thing, and eventually wished we put those extras into standalone packages, because making a breaking change e.g. in the email connector requires a semver-major release of entire loopback now.

---

Regardless of the approach we decide to take, I'd like to propose to keep working incrementally in baby steps, making one change at a time. This will make it easier to review and land the pull requests, and if our priorities later happen to shift elsewhere, then we will have at least part of this work done done.

[lehni]

@bajtos I agree with all of this. Just quickly to explain the monkey-patching part:

1. Transaction is defined here in loopback-connector:
   https://github.com/strongloop/loopback-connector/blob/f2cbdedee5800c33a977be8d58f5c8b1a30143d3/lib/transaction.js#L21-L25

2. Transaction#commit() and Transaction#rollback() are defined here in loopback-connector:
   https://github.com/strongloop/loopback-connector/blob/f2cbdedee5800c33a977be8d58f5c8b1a30143d3/lib/transaction.js#L43-L59

3. Various Transaction methods are monkey-patched, "promisified" and "observeified" here in loopback-datasource-juggler:
   https://github.com/strongloop/loopback-datasource-juggler/blob/f18d3487c64ea1ed3896da4d7a5010eadbd59a0d/lib/transaction.js#L115-L219

And I don't think that's good practise, but a recipe for confusion and disaster :)

[lehni]

@bajtos so I'd like to tackle this one now.

Perhaps. The downside of such module is that over time, it may become a kitchen sink for utility functions, and then we will have hard times making breaking changes, because everybody will be depending on this module. We went through a similar process with loopback package - we put auth, offline-sync, email, etc. there, because it was the easiest thing, and eventually wished we put those extras into standalone packages, because making a breaking change e.g. in the email connector requires a semver-major release of entire loopback now.

So that's a clear argument for a separate loopback-observer then? But have a look at the ObserverMixin maybe... There is very little actual code in there. Does it really merit a separate package?

The other option is loopback-utils, with just ObserverMixin and utils.createPromiseCallback() for now. And the convention will be that only stabilised utility functions and classes go there that barely change anymore?

Either way, how would I go about creating such a package?

[bajtos]

Perhaps. The downside of such module is that over time, it may become a kitchen sink for utility functions, and then we will have hard times making breaking changes, because everybody will be depending on this module. We went through a similar process with loopback package - we put auth, offline-sync, email, etc. there, because it was the easiest thing, and eventually wished we put those extras into standalone packages, because making a breaking change e.g. in the email connector requires a semver-major release of entire loopback now.

So that's a clear argument for a separate loopback-observer then?

Yes :)

But have a look at the ObserverMixin maybe... There is very little actual code in there. Does it really merit a separate package?

That's always the problematic part of "many small modules" philosophy.

The other option is loopback-utils, with just ObserverMixin and utils.createPromiseCallback() for now. And the convention will be that only stabilised utility functions and classes go there that barely change anymore?

I guess that's a reasonable compromise 👍 Considering that the next major version of LoopBack will be based on entirely new codebase (see https://github.com/strongloop/loopback-next), I think the overhead of creating multiple smaller packages (one for the observer, another for other utility functions) is not worth the effort at this moment.

@raymondfeng @kjdelisle what's your opinion?

Either way, how would I go about creating such a package?

One of GitHub org admins like me, @kjdelisle or @raymondfeng need to create a new GH repository and setup permissions so that you have write access (I think we should give write access to everybody who has write access to loopback, juggler and remoting now.) Then you can start the usual pull-request based development.

We need to agree on the package name - is loopback-util the name we want? (I am fine with that.)

[lehni]

Sounds all good! I would suggest using the plural loopback-utils. I shall wait to hear back from the others, then.

[lehni]

@raymondfeng @kjdelisle @bajtos @raymondfeng I'm hoping for feedback here so we can wrap this up. Please let me know your thoughts, thanks!

[kjdelisle]

@lehni I'm good with the plural form (we already do it for things like strong-tools, so it makes sense to me).
I'll create the empty repository for it and give you read/write.

[kjdelisle]

@lehni https://github.com/strongloop/loopback-utils/ 🎉

[lehni]

@kjdelisle thanks! I'm about to leave on a mini-holiday, will start working on this when back next week.

[bajtos]

@lehni no worries, take your time. When you get to this work, could you please start with a pull request setting up the project infrastructure? Mocha tests, eslint, Travis config, etc. I'll enable Travis CI builds right now.

[lehni]

@kjdelisle @bajtos I think we have a naming clash 😞

https://www.npmjs.com/package/loopback-utils

What to do?

[lehni]

Other than that, I think my code is ready for review at https://github.com/strongloop/loopback-utils/tree/feature/add-utils-from-juggler

[lehni]

As for the naming issue... What about loopback-helpers instead of loopback-utils?

Another option is loopback-tools or loopback-basics but they're all kinda wrong.

[bajtos]

loopback-helpers sounds good to me. Few more options to consider: loopback-common, loopback-core-utils.

@lehni let me know what name do you like best, so that I can rename the new repo.

[lehni]

I like loopback-common better than loopback-helpers. Another options is loopback-shared. loopback-core-utils is very descriptive which is good, but then I expect more from the repo than what it currently provides. Which one do you think is better, loopback-shared or loopback-common?