I also went back and forth on 400 vs 422 but ultimately concluded that 400 is the correct one to use here. My understanding is that 422 is meant to address semantic issues in the request as opposed to 400 which is typically used for invalid formatted input.
As Dan mentioned, in this case the server is actually unable to identify if the request was well formed, since it does not even have an understanding of if the requirement/update is valid. If the server is not able to have a semantic understanding of this requirement/update, then I think from a server perspective it must be concluded that it is invalid input. >(Any other options considered?) I didn't really consider any other high level options tbh since I think they lead to bad behaviors, but for the sake of thinking it through explicitly: The options for handling unknown input range from ignoring it to spec'ing out the required failure mode (this proposal). If servers ignored the update rather than fail, then clients would think their operations would succeed but they really didn't. If a server ignored a requirement it couldn't understand, that could lead to correctness issues! As a result, I think the clearest option is to have a defined failure mode. Thanks, Amogh Jahagirdar On Mon, Aug 5, 2024 at 9:22 AM Daniel Weeks <dwe...@apache.org> wrote: > I feel like this is a little bit of a gray area in terms of 400 vs 422. > While I agree that 422 reads like the right answer just based on the > definition of the codes, I think that it will be hard to implement and may > not make sense in context of how the server evolves. > > If a server has not implemented a new update/requirement, then it would be > seen as a bad/request (400) because the server doesn't have the ability to > determine whether the request is well formatted or not (it knows nothing > about the request). > > I would say the 422 makes sense if the server understands and can parse > the request, but cannot complete the request, which would not be the case. > It would also be hard for a service to determine if the request was well > formed if it does not know about the request. > > I think 400 makes the most sense here. > > -Dan > > On Fri, Aug 2, 2024 at 4:28 PM Dmitri Bourlatchkov > <dmitri.bourlatch...@dremio.com.invalid> wrote: > >> I'd suggest using error code 422 (Unprocessable Content) [1] instead. >> >> 400 generally means the request was not well-formed (e.g. syntax error, >> or using invalid chars, etc.). However, here we have a situation when a >> client actually sends a valid request, but the server is not able to >> properly process some part of it. >> >> Cheers, >> Dmitri. >> >> [1] https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/422 >> >> On Fri, Aug 2, 2024 at 1:15 PM Amogh Jahagirdar <2am...@gmail.com> wrote: >> >>> Hey everyone, >>> >>> I'm starting this thread to discuss clarifying in the REST Spec on what >>> servers should do >>> if as part of a commit operation they receive an update >>> <https://github.com/apache/iceberg/blob/main/open-api/rest-catalog-open-api.yaml#L2570C5-L2570C16> >>> or requirement >>> <https://github.com/apache/iceberg/blob/main/open-api/rest-catalog-open-api.yaml#L2601> >>> that >>> is unknown. >>> >>> What I am proposing is to clearly say in the spec that if a server >>> receives an update or requirement which is unknown it must fail with a 400 >>> error code. 400 seems the most appropriate code to be able to >>> describe unknown input in the commit table request. Here's the PR >>> https://github.com/apache/iceberg/pull/10848/files >>> >>> This is a simple way to ensure that implementations avoid correctness >>> issues that come from ignoring unknown updates/requirements. It also >>> enables the community to be able to add new operations without any complex >>> versioning schemes. >>> >>> A more concrete example: Recently there's been work on removing >>> inactive partition specs <https://github.com/apache/iceberg/pull/10755>from >>> table metadata. With this proposal, any server which receives a >>> RemovePartitionSpecUpdate and does not recognize it, must fail with a 400. >>> >>> Note: I will propose the specific example of RemovePartitionSpecUpdate >>> spec change in a different thread. >>> >>> Thanks, >>> >>> Amogh Jahagirdar >>> >>
