search

getodk / aggregate

ODK Aggregate is a Java server that stores, analyzes, and presents survey data collected using ODK Collect. Contribute and make the world a better place! ✨🗄✨
https://docs.opendatakit.org/aggregate-intro/
Other
74 stars 227 forks source link

allow any 2XX status code from an external JSON server #469

Closed terencehonles closed 4 years ago

terencehonles commented 5 years ago

When a server responds with another 2XX code, such as 201 the server treats this as an error. This accepts any 2XX response.

I used the number 300 instead of SC_MULTIPLE_CHOICES as that constant's name doesn't signify the start of the non success range.

What has been done to verify that this works as intended?

We'll be deploying this shortly.

Why is this the best possible solution? Were any other approaches considered?

Any 2XX code is a success code.

Are there any risks to merging this code? If so, what are they?

No.

Do we need any specific form for testing your changes? If so, please attach one

No

Does this change require updates to documentation? If so, please file an issue at https://github.com/opendatakit/docs/issues/new and include the link below.

ggalmazor commented 5 years ago

I think this is safe to be merged directly. I don't know how we could test this manually anyway. Do you agree, @lognaturel?

yanokwa commented 5 years ago

I'm not the world's greatest API designer or anything, but seems like we should be narrower here and only allow 200, 201, and 202. I don't feel strongly about it...

terencehonles commented 5 years ago

From https://www.iana.org/assignments/http-status-codes/http-status-codes.xhtml

2xx: Success - The action was successfully received, understood, and accepted

200 | OK | [RFC7231, Section 6.3.1] 201 | Created | [RFC7231, Section 6.3.2] 202 | Accepted | [RFC7231, Section 6.3.3] 203 | Non-Authoritative Information | [RFC7231, Section 6.3.4] 204 | No Content | [RFC7231, Section 6.3.5] 205 | Reset Content | [RFC7231, Section 6.3.6] 206 | Partial Content | [RFC7233, Section 4.1] 207 | Multi-Status | [RFC4918] 208 | Already Reported | [RFC5842] 209-225 | Unassigned |   226 | IM Used | [RFC3229] 227-299 | Unassigned

200, 201, 202, 204, 207, and 208 should be acceptable, but according to the document all of the 200 range is a "success" and there probably is not a strong reason to reject the couple that fall through and the unassigned ranges just because they don't "seem" to make sense. If the server received, understood, and accepted the request that's pretty much all the ODK server cares about.

ggalmazor commented 5 years ago

I agree with @terencehonles. I think it's safe to assume any incoming 2xx response completes successfully the request.