Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Constraint validation doesn't seem to ensure that the validated value is of the particular type #3976

Closed
MaryamZi opened this issue Jan 24, 2023 · 2 comments · Fixed by ballerina-platform/module-ballerina-constraint#52
Assignees
Labels
module/constraint Points/1 Priority/High Reason/EngineeringMistake The issue occurred due to a mistake made in the past. Team/PCM Protocol connector packages related issues Type/Bug
Milestone

Comments

@MaryamZi
Copy link
Member

MaryamZi commented Jan 24, 2023

Description:
$title, may have to convert the value in the Java implementation.

Steps to reproduce:

import ballerina/constraint;
import ballerina/io;

type User record {|
    int id;
    @constraint:String {
        minLength: 2
    }
    string name;
|};

public function main() returns error? {
    json j = {
        "id": 1,
        "name": "Joy"
    };

    User|error user = constraint:validate(j);
    if user !is error {
        // validation was successful, `user` will be of type `User` here
        io:println(user is User); // expected true, prints false
        io:println(user === j); // expected false, prints true, value is still the original JSON object
    }
}

Affected Versions:
2201.3.2

@TharmiganK TharmiganK self-assigned this Jan 24, 2023
@TharmiganK TharmiganK added the Team/PCM Protocol connector packages related issues label Jan 24, 2023
@TharmiganK
Copy link
Contributor

TharmiganK commented Jan 30, 2023

With the current implementation we only use the typedesc from the validate() function, to infer the constraint annotations. We could improve this behavior by converting the value to the contextually expected type(using CloneWithType.convert(type, value)), only when the type of the value is different from the contextually expected type. This should solve this issue.

The conversion will be done before the validation to avoid unnecessary validation of unmatched type and value.

With the above fix suggestion, we could expect the following output:

import ballerina/constraint;
import ballerina/io;

type User record {|
    int id;
    @constraint:String {
        minLength: 2
    }
    string name;
|};

public function main() returns error? {
    json j1 = {"id": 1, "name": "Joy"};
    // j1 will be cloned with User type
    User user1 = check constraint:validate(j1);
    io:println(user1 is User); // prints true
    io:println(user1 === j1); // prints false

    User user = {"id": 1, "name": "Joy"};
    // user will not be cloned
    user = check constraint:validate(user);
    io:println(user is User); // prints true

    // user will not be cloned 
    User validation = check constraint:validate(user);
    io:println(validation is User); // prints true
    io:println(validation === user); // prints true

    json j2 = {"id": 1, "username": "Joy"};
    User _ = check constraint:validate(j2); // Returns an error
}

@github-actions
Copy link

This issue is NOT closed with a proper Reason/ label. Make sure to add proper reason label before closing. Please add or leave a comment with the proper reason label now.

      - Reason/EngineeringMistake - The issue occurred due to a mistake made in the past.
      - Reason/Regression - The issue has introduced a regression.
      - Reason/MultipleComponentInteraction - Issue occured due to interactions in multiple components.
      - Reason/Complex - Issue occurred due to complex scenario.
      - Reason/Invalid - Issue is invalid.
      - Reason/Other - None of the above cases.

@TharmiganK TharmiganK added the Reason/EngineeringMistake The issue occurred due to a mistake made in the past. label Jan 31, 2023
@TharmiganK TharmiganK added this to the 2201.4.0 milestone Feb 2, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
module/constraint Points/1 Priority/High Reason/EngineeringMistake The issue occurred due to a mistake made in the past. Team/PCM Protocol connector packages related issues Type/Bug
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants