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

Fix a bug when running patch queries #6

Merged
merged 1 commit into from
Sep 28, 2017
Merged

Conversation

dsego
Copy link
Contributor

@dsego dsego commented Sep 23, 2017

This fixes a bug where running a patch query would report an error for the wrong field.

I encountered the bug with the canonical example in my model.

const Unique = require('objection-unique')({
  fields: ['username', 'email'],
  identifiers: ['id'],
});

When trying to patch only email or username, I would get the wrong validation error.

User
  .query()
  .findById(req.params.id)
  .throwIfNotFound()
  .then(user => user.$query()
    .patchAndFetch(data)
    .then(patchedUser => res.send(patchedUser)))
  .catch(err => next(err));

screen shot 2017-09-24 at 12 33 55 am

This is the test failing, before the fix.
screen shot 2017-09-24 at 12 30 19 am

@nunorafaelrocha
Copy link
Collaborator

nunorafaelrocha commented Sep 25, 2017

Hi @dsego

Thanks for the PR. Can you please do a rebase interactive in your branch to apply that fixup you have.

About the test that was failing before the fix - I run the tests on master branch and they all passed with 100% coverage. Could you clarify the conditions in which that happened?

Thanks!

This fixes a bug where running a patch query would report an error for
the wrong field.
@dsego
Copy link
Contributor Author

dsego commented Sep 25, 2017

HI, that's the new test I wrote failing before the bug-fix.

EDIT: To be more precise, I encountered a bug, wrote a test to reproduce it, confirmed it fails, and then wrote a fix.

@nunorafaelrocha nunorafaelrocha merged commit a9e948d into seegno:master Sep 28, 2017
@nunorafaelrocha
Copy link
Collaborator

nunorafaelrocha commented Sep 28, 2017

@dsego Thanks for your PR, a new version was already published.

Cheers

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