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鈥檒l 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

Projects
None yet
5 participants
@bajtos
Copy link
Member

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 requested review from jannyHou, b-admike and nabdelgadir Apr 15, 2019

@bajtos bajtos requested a review from raymondfeng as a code owner Apr 15, 2019

@bajtos bajtos self-assigned this Apr 15, 2019

}
}
return constrainedData;
return originalData.map(obj => constrainDataObject(obj, constraint));

This comment has been minimized.

Copy link
@bajtos

bajtos Apr 15, 2019

Author Member

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 Apr 15, 2019

@b-admike
Copy link
Member

left a comment

LGTM 馃挴 .

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

@jannyHou
Copy link
Contributor

left a comment

LGTM

@bajtos bajtos force-pushed the feat/relax-constraint-check branch from 42948aa to ec176db Apr 16, 2019

fix(repository): relax constrain check to allow input containing cons鈥
鈥rained 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 Apr 16, 2019

@bajtos bajtos merged commit c774ed1 into master Apr 16, 2019

3 of 4 checks passed

coverage/coveralls Coverage decreased (-0.002%) to 91.243%
Details
clahub All contributors have signed the Contributor License Agreement.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bajtos bajtos deleted the feat/relax-constraint-check branch Apr 16, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.