[SEMVER-MAJOR] throw error for undefined mixin by alexpit · Pull Request #944 · loopbackio/loopback-datasource-juggler · GitHub

alexpit · 2016-05-24T18:21:45Z

connect to #566

alexpit · 2016-05-24T20:36:19Z

@bajtos, please review. thanks

richardpringle · 2016-05-25T13:31:19Z

@alexpit, don't forget to update the release notes!

bajtos · 2016-05-25T14:09:51Z

      modelClass.mixin(model, options);
    } else {
      debug('Invalid mixin: %s', name);
+      throw new Error('Invalid mixin: ' + name);


Please include the model name in the error message, otherwise it's impossible to find the model which is referencing this missing mixing.

I would also reword the message a bit, e.g.

'Model ' + modelClass.modelName + ' uses unknown mixin ' + name

bajtos · 2016-05-25T14:12:44Z

@alexpit thank you for the pull request, you are headed in the right direction! I left few comments to address, see above.

alexpit · 2016-05-25T17:05:37Z

@richardpringle, please review. thx

alexpit · 2016-05-25T17:05:57Z

@bajtos, please review addressed comments. thx

richardpringle · 2016-05-25T17:49:10Z

+silently ignored. This has been fixed in 3.0, therefore an undefined mixin
+applied to a LoopBack model will throw an error that needs to be handled.
+
+Please see [related code change](https://github.com/strongloop/loopback-datasource-juggler/pull/944)


Nit pick:

Please see [related code change]() for details.

bajtos · 2016-05-26T08:35:44Z

    var model = this.modelBuilder.getModel(name);
    if (model) {
-      debug('Mixin is resolved to a model: %s', name);
+      debug('Mixin "' + name + '" resolves to model: %s', modelClass.modelName);


This is not correct. Mixin "name" is resolved to a model called "name" in this branch, and then it is applied to another model called "modelClass.modelName".

It's better to avoid making unrelated code changes, as it makes code archaeology more difficult.

Please revert this line.

alexpit · 2016-05-26T13:57:47Z

@bajtos, please review addressed comments. thx

bajtos · 2016-05-27T13:13:25Z

LGTM 👏

Please squash the commits before landing, the commands are git rebase -i master and git push -f.

Amir-61 · 2016-05-29T02:09:07Z

Hey @bajtos and @alexpit,

@bajtos Does Alex need to backprot this change?

@alexpit For loopback-datasource-juggler, we may need to backport the changes to 2.x branch as well; please see our team documentation. If @bajtos confirmed that you need to backport the changes, please let me know if you need any help for backproting and I can walk you through the steps.

Thanks :-)

richardpringle · 2016-05-29T05:22:02Z

Woah, dependent loopback failed, that can't be good. Also, @alexpit, i'll help you with squashing.

@bajtos, git push -f? That will force push master. Should be git push origin +branch_name no?

Amir-61 · 2016-05-29T05:25:50Z

@bajtos, git push -f? That will force push master. Should be git push origin +branch_name no?

I believe Miroslav meant run git push -f when you are in your feature branch and after rebasing and squashing the commits in feature branch.

richardpringle · 2016-05-29T05:32:05Z

I believe that depends on your push policy configuration. It's safer to explicitly push a certain branch with '+' causing the force push.

bajtos · 2016-05-30T15:14:53Z

Does Alex need to backprot this change?

No, we cannot back-port breaking changes.

git push -f? That will force push master. Should be git push origin +branch_name no?

I believe Miroslav meant run git push -f when you are in your feature branch and after rebasing and squashing the commits in feature branch.

I believe that depends on your push policy configuration. It's safer to explicitly push a certain branch with '+' causing the force push.

Interesting. I think it's rather unlikely that one would one end up with a feature branch configured to be pushed to a long-lived remote branch like master, but then there is no harm in being cautious.

BTW in some repositories, we have locked long-lived branches like master and 2.x to reject force-pushes. If you find a repository that does not have master locked, then feel free to make the change yourself.

bajtos · 2016-05-30T15:15:49Z

@slnode test please

alexpit · 2016-05-31T15:41:48Z

@bajtos please review

bajtos · 2016-05-31T16:06:36Z

PR Linter — PR out of date, needs to be rebased

@alexpit could you please rebase on top of the current master?

alexpit · 2016-05-31T16:15:06Z

@slnode test please

alexpit added the #review label May 24, 2016

alexpit force-pushed the fix-mixin-err-handler branch from c8f4246 to b1285ce Compare May 24, 2016 20:24

alexpit assigned alexpit and bajtos and unassigned alexpit May 24, 2016

bajtos reviewed May 25, 2016
View reviewed changes

bajtos assigned alexpit and unassigned bajtos May 25, 2016

richardpringle reviewed May 25, 2016
View reviewed changes

richardpringle assigned bajtos and unassigned alexpit May 25, 2016

bajtos reviewed May 26, 2016
View reviewed changes

bajtos assigned alexpit and unassigned bajtos May 26, 2016

bajtos changed the title ~~fix error handling when applying undefined mixins~~ [SEMVER-MAJOR] fix error handling when applying undefined mixins May 27, 2016

bajtos changed the title ~~[SEMVER-MAJOR] fix error handling when applying undefined mixins~~ [SEMVER-MAJOR] throw an error when applying undefined mixin May 27, 2016

alexpit changed the title ~~[SEMVER-MAJOR] throw an error when applying undefined mixin~~ [SEMVER-MAJOR] throw error for undefined mixin May 30, 2016

alexpit force-pushed the fix-mixin-err-handler branch from 3547299 to e0e31ca Compare May 30, 2016 18:52

fix error handling when applying undefined mixins

423db34

alexpit force-pushed the fix-mixin-err-handler branch from e0e31ca to 423db34 Compare May 31, 2016 16:26

alexpit merged commit 5b02507 into master May 31, 2016

alexpit deleted the fix-mixin-err-handler branch May 31, 2016 19:31

alexpit mentioned this pull request May 31, 2016

Throw an error when model uses unknown mixin #566

Closed

Conversation

alexpit commented May 24, 2016 • edited by richardpringle Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexpit commented May 24, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richardpringle commented May 25, 2016

Uh oh!

bajtos May 25, 2016

Choose a reason for hiding this comment

Uh oh!

bajtos commented May 25, 2016

Uh oh!

alexpit commented May 25, 2016

Uh oh!

alexpit commented May 25, 2016

Uh oh!

richardpringle May 25, 2016

Choose a reason for hiding this comment

Uh oh!

bajtos May 26, 2016

Choose a reason for hiding this comment

Uh oh!

alexpit commented May 26, 2016

Uh oh!

bajtos commented May 27, 2016

Uh oh!

Amir-61 commented May 29, 2016

Uh oh!

richardpringle commented May 29, 2016 • edited by bajtos Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Amir-61 commented May 29, 2016

Uh oh!

richardpringle commented May 29, 2016

Uh oh!

bajtos commented May 30, 2016

Uh oh!

bajtos commented May 30, 2016

Uh oh!

alexpit commented May 31, 2016

Uh oh!

bajtos commented May 31, 2016

Uh oh!

alexpit commented May 31, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

alexpit commented May 24, 2016 •

edited by richardpringle

Loading

alexpit commented May 24, 2016 •

edited

Loading

richardpringle commented May 29, 2016 •

edited by bajtos

Loading