[@azure/core-http]/[@azure/core-client] - Remove Constraints Check

[sarangan12]

Packages impacted by this PR

@azure/core-http (For Track 1) & @azure/core-client (For Track 2). Thus, by extension, all the packages dependent on these package.

Issues associated with this PR

#21839

Describe the problem that is addressed by this PR

Basically, the problem started with this issue. One of the customers reported that there is an issue with a way multipleOf constraint is handled. That is because of the % operator. While working through this, we have decided that the constraints check can be removed altogether. I have already confirmed with other language owners that they do not perform constraints check.

What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?

There are 2 design options.

Option 1

Remove the constraints method altogether. This will include removal of method from Serializer Interface and will cause breaking changes.

Option 2

Just do not call the constraints method. This will not cause break in code. But there is a slight possibility that this might cause break in behavior for custom serializers. But, that can be justified with reason that we are not going to perform any constraints validation.

So, I have decided to go with option 2.

Are there test cases added in this PR? (If not, why?)

No. Existing test cases are fine.

Provide a list of related PRs (if any)

None

Checklists

• Added impacted package name to the issue description
• Does this PR needs any fixes in the SDK Generator?** (If so, create an Issue in the Autorest/typescript repository and link it here)
• Added a changelog (if necessary)

@xirzec Please review and approve the PR.

[azure-sdk]

API change check

API changes are not detected in this pull request.

[xirzec]

Seems reasonable to me, no point in doing extra validation especially if we're the only language who does.

@willmtemple @joheredi - going to add you as additional reviewers. I don't think this change will be contentious, but I'd like us all to agree on that. 😄

[jeremymeng]

Makes sense. LGTM

[deyaaeldeen]

I am happy with disabling constraint checking because I was burnt by it before in Schema Registry and had to remove the constraints from the swagger for the client to work properly: #18666.

However, I am on the fence on the taken approach to do so since constraint checking is part of the public surface of the core libraries as Sarangan pointed out. Disabling constraint checking in core but leaving its knobs in the public surface could be confusing or even misleading. Tagging them with @deprecated could help alleviate this but I am not sure if it is ok to for a deprecated feature to be outright disabled.

An alternative approach is to update the code generator to not add constraints in the generated mappers as discussed in Azure/autorest.typescript#829 but then not every library will enjoy the benefit unless it gets regenerated.

I don't have strong opinions either way but I just wanted to point it out.

[xirzec]

However, I am on the fence on the taken approach to do so since constraint checking is part of the public surface of the core libraries as Sarangan pointed out. Disabling constraint checking in core but leaving its knobs in the public surface could be confusing or even misleading. Tagging them with @deprecated could help alleviate this but I am not sure if it is ok to for a deprecated feature to be outright disabled.

I think from a correctness POV, you're right, but pragmatically nobody is relying on constraint checking failing today, so the runtime behavior should be largely unaffected for SDKs we've shipped.

[jeremymeng]

We are seeing some KeyVault test failures after this commit. It's a negative test of passing empty values. Now previously expected validation error is gone, and the test got a new error, which is not very helpful, but probably fine since this is an error case. /cc @timovv

[xirzec]

We are seeing some KeyVault test failures after this commit. It's a negative test of passing empty values. Now previously expected validation error is gone, and the test got a new error, which is not very helpful, but probably fine since this is an error case. /cc @timovv

I'd be fine if we wanted to add some custom error handling in the client to make up for whatever unhelpful thing the service sends back

[abhinav-ghai]

We are also seeing failure in Digital Twins models creation path, since that is checking for MinItems constraint while passing empty values.

[sarangan12]

@abhinav-ghai Could you please clarify if the failure is in the test code? Or is it in Production code/Customer Code? In such a case, Could you explain the scenario?