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: relax constrain check to allow input containing constrained values #2754

Merged
merged 1 commit into from
Apr 16, 2019

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Apr 15, 2019

Consider a Product constraint like {categoryId: 1} and a request
to update (patch) Product with the following data:

{
  name: 'updated',
  categoryId: 1
}

Before this change, such request would be incorrectly rejected.

With this change in place, such requests are allowed.

Fixes #1719, resolves #2658

/cc @foobarrrz

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

@bajtos bajtos added bug developer-experience Issues affecting ease of use and overall experience of LB users Repository Issues related to @loopback/repository package labels Apr 15, 2019
@bajtos bajtos requested a review from raymondfeng as a code owner April 15, 2019 11:54
@bajtos bajtos self-assigned this Apr 15, 2019
}
}
return constrainedData;
return originalData.map(obj => constrainDataObject(obj, constraint));
Copy link
Member Author

Choose a reason for hiding this comment

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

This is slightly changing the behavior when the input data is trying to modify the constrained properties. Before my change, we would silently replace values provided by the user with values enforced by the constraint. With my change in place, we reject requests trying to modify constrained properties.

AFAICT, the function constrainDataObjects (notice the plural name!) is not used anywhere in loopback-next codebase and I would find it surprising if anybody was using it directly from 3rd party projects. I consider this change as a backwards-compatible bug fix (not a breaking change).

@bajtos bajtos force-pushed the feat/relax-constraint-check branch from a807538 to 42948aa Compare April 15, 2019 12:12
Copy link
Contributor

@b-admike b-admike left a comment

Choose a reason for hiding this comment

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

LGTM 💯 .

EDIT: Nitpick on commit message fix: -> fix(repository):

Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

LGTM

@bajtos bajtos force-pushed the feat/relax-constraint-check branch from 42948aa to ec176db Compare April 16, 2019 06:49
…trained values

Consider a Product constraint like `{categoryId: 1}` and a request
to update (patch) Product with the following data:

    {
      name: 'updated',
      categoryId: 1
    }

Before this change, such request would be incorrectly rejected.

With this change in place, such requests are allowed.
@bajtos bajtos force-pushed the feat/relax-constraint-check branch from ec176db to 21a5f82 Compare April 16, 2019 06:49
@bajtos bajtos merged commit c774ed1 into master Apr 16, 2019
@bajtos bajtos deleted the feat/relax-constraint-check branch April 16, 2019 07:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug developer-experience Issues affecting ease of use and overall experience of LB users Repository Issues related to @loopback/repository package
Projects
None yet
5 participants