Update addError to give precedence to newly added schemas

[Avaq]

Type

• Refactor
• Feature
• Bug Fix
• Optimization
• Documentation Update

Description

HttpApiSchema.UnionUnify creates a union whose members are listed in the order of [...firstArgUnionMembers, ...secondArgUnionMembers]. When encoding a union, Schema will try each member in the order that they appear.

So the addError method on each of the HttpApi* classes, by passing the user-provided schema as a second argument, causes older schemas that encompass newer schemas to take precedence over the newer ones.

By flipping the argument order, users are now able to use addError with a schema that (partly) encompasses an older one, to override the encoding behaviour for values conforming to those schemas.

Related

• Related to Allow HttpApiEndpoint to override HttpApiDecodeError response #4751 (but doesn't fully fix it, as the default HttpApiDecodeError schema is still part of every HttpApi)
• Closes Customizing HttpApiDecodeError responses #5465

[changeset-bot]

⚠️ No Changeset found

Latest commit: 8ac3f22

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[Avaq]

I also have a WIP for adding a new .setDecodeError method to each of the HttpApi* classes, which would fix #5465, as well as make this ☝️ proposed "work around" for #5465 obsolete.

However, since this already addresses the bulk of the pain with a very small change, and since I think it's a good change overall, I'd say it's still worth considering.

[Avaq]

There's more sites where a union of the self.errorSchema is made with some the error schema of a unit being added. Like with addMiddleware. I'm not sure how we should treat those, but it may be worth considering flipping the arg order there too.

For example, if the middleware failure is a schema that encompasses a type that was already part of the app, I guess it makes sense to do the encoding with the middleware-supplied schema? But I can see a counter-argument to that too.

If we don't want to flip the arg order, an alternative solution in the same vain as this one, would be to have addError filter out any HttpApiDecodeError schemas from the union members while "unionifying" them, and then tag it back on at the end.

[Avaq]

For some reason if I install the latest main version (e8bd5b8) of Effect Platform, the problem that this PR is trying to address goes away on its own! 🤯

From reading the code, I don't really understand what changed it, but it even fixes the OpenAPI specs (which this PR doesn't).

[Avaq]

it even fixes the OpenAPI specs

Actually no. The tagged error is still in the spec, but it's kind of hidden on the swagger page behind my custom 400-errors.

[tim-smart]

I think the order that a user adds their error schemas should be preserved (if they call .addError multiple times etc).

[Avaq]

@tim-smart The order of .addError calls is still important, but this PR reverses it, so newer calls take precedence.

The idea being that whoever attaches an error later, has knowledge of what came before, and so can make an informed decision about how to override previous encoders.

To me, it makes more intuitive sense than the old behaviour where old codecs override newer ones. For example, now if I do .addError(Schema.Any), I'm effectively suppressing any future error codecs that could be added, because all values will be encoded by the Any schema.

That's essentially what is was(?) happening with the first error that's added to all HttpApis: The HttpApiDecodeError, making it impossible to encode such values in any other way.

I'm not sure about the mechanism by which this no longer seems to be the case on e8bd5b8 though. If it's intentional, then we can close this PR. I'll still work on a separate PR to make the default HttpApiDecodeError completely replaceable in the spec though, so it also doesn't show up in Swagger docs and whatnot.

[tim-smart]

To me, it makes more intuitive sense than the old behaviour where old codecs override newer ones. For example, now if I do .addError(Schema.Any), I'm effectively suppressing any future error codecs that could be added, because all values will be encoded by the Any schema.

I don't think so, as a "catch-all" schema should be processed last, after all the previous schemas.

Adding the system level schemas could probably be done last however.

[gcanti]

What about a replaceError(f: (prev: Schema | undefined) => Schema | undefined) API instead of addError? This would let the user decide what comes first, while system-added schemas would always be placed at the end.

[Avaq]

@gcanti @tim-smart I opened an alternative PR a few weeks back that actually addresses the issue more directly and without drawbacks as far as I can see: #5654
Can you please take a look? It would allow me to finally move forward incorporating Effect Platform into my project 🥺