ASCOMInitiative / ConformU

ConformU is a cross-platform tool to validate that Alpaca Devices and ASCOM Drivers conform to the ASCOM interface specification. It supersedes the original Windows based Conform application.
10 stars 2 forks source link

PUT params with invalid casing #6

Closed RReverser closed 1 year ago

RReverser commented 1 year ago

I think I found one more potentially conflicting statement between the spec and ConformU / simulators.

One (last?) test my server library is currently failing is that ConformU expects server to return a Bad Request error when key casing in PUT params differs from the standard one. That seems a bit odd given that PUT params are supposed to be matched in case-sensitive order, and unknown params should be ignored.

I thought there might be two possibilities why an error would be expected in that case: cLiEnTiD​ is treated as any other unrecognised property and unrecognised properties should cause the server to error out. However the spec says "Alpaca clients and devices should not return errors when unrecognised parameters are received." so this shouldn't be it. The spec doesn't say so, but perhaps the server is expected to search for PUT params in case-insensitive manner, but then validate them in case-sensitive manner, so that cLiEnTiD=1​ has to be recognised as ClientID​ but then errored on because the case does not match.

I decided to check which one it is by running same ConformU test ("Check Alpaca protocol") against the default configuration of ASCOM Alpaca Simulators, and I see it's failing with the same violations:

PUT SlewToAltAz ==> Parameter Azimuth (Bad casing) - Expected HTTP status : 400 (BadRequest) but received status: 200 (OK).
  Response: {"ClientTransactionID":67890,"ServerTransactionID":427,"ErrorNumber":0,"ErrorMessage":""}

PUT SlewToAltAz ==> Parameter Altitude (Bad casing) - Expected HTTP status : 400 (BadRequest) but received status: 200 (OK).
  Response: {"ClientTransactionID":67890,"ServerTransactionID":429,"ErrorNumber":0,"ErrorMessage":""}

PUT SyncToAltAz ==> Different ClientId case - Expected HTTP status : 400 (BadRequest) but received status: 200 (OK).
  Response: {"ClientTransactionID":67890,"ServerTransactionID":435,"ErrorNumber":0,"ErrorMessage":""}

PUT SyncToAltAz ==> Different ClientTransactionId case - Expected HTTP status : 400 (BadRequest) but received status: 200 (OK).
  Response: {"ClientTransactionID":67890,"ServerTransactionID":436,"ErrorNumber":0,"ErrorMessage":""}

PUT SyncToAltAz ==> Parameter Azimuth (Bad casing) - Expected HTTP status : 400 (BadRequest) but received status: 200 (OK).
  Response: {"ClientTransactionID":67890,"ServerTransactionID":437,"ErrorNumber":0,"ErrorMessage":""}

PUT SyncToAltAz ==> Parameter Altitude (Bad casing) - Expected HTTP status : 400 (BadRequest) but received status: 200 (OK).
  Response: {"ClientTransactionID":67890,"ServerTransactionID":438,"ErrorNumber":0,"ErrorMessage":""}

...

Between the spec and the simulators being both permissive and ignoring PUT params with invalid casing, it seems that ConformU test is the one in the wrong here? If not, can you please advise what the correct behaviour is and update the spec and the simulators?

Peter-Simpson commented 1 year ago

Hi Ingvar,

The Altitude and Azimuth responses look correct to me because, if the parameter is incorrectly cased and the device ignores it, the device does not know what altitude or azimuth the client wants to use. This situation constitutes an "incomplete" request from the client because a mandatory input value is missing, hence a 400 error response from the device is appropriate.

ClientId and ClientTransactionId are different because they are optional parameters and the device can still carry out the requested operation when they are missing either because they were never sent or because they are not correctly cased.

Off the top of my head I can't think of a way to test for ClientId issues because this value is not required to be round-tripped by the device, but I think there may be another way to deal with ClientTransactionId, which is required to be round-tripped by the device. This would be a change to ConformU.

Best wishes, Peter

RReverser commented 1 year ago

The Altitude and Azimuth responses look correct to me because, if the parameter is incorrectly cased and the device ignores it, the device does not know what altitude or azimuth the client wants to use. This situation constitutes an "incomplete" request from the client because a mandatory input value is missing, hence a 400 error response from the device is appropriate.

Yes, sorry, I should have been more specific and included only optional properties in the log.

I can't think of a way to test for ClientId issues because this value is not required to be round-tripped by the device

Yeah I don't see any solution except removing that test.

This would be a change to ConformU.

Ok, so this is a problem in ConformU test not in the spec & the simulator and it's okay to ignore ClientID & ClientTransactionID with invalid casing and return HTTP 200? Sounds great!

Peter-Simpson commented 1 year ago

Hi Ingvar,

I've revised ConformU to acknowledge that ClientID and ClientTransactionID are both optional and should be accepted and used if mis-cased in a query parameter and ignored if mis-cased in a form parameter. I've also added checks to confirm that the ClientTransactionID value is correctly round-tripped by the device.

Please can you let me know whether this version now works as expected?

ConformU(1.3.0.15153)Setup.exe

Many thanks, Peter

RReverser commented 1 year ago

Hi Peter, thanks, but the link above is broken.

Peter-Simpson commented 1 year ago

Oops sorry, missed out the most of the URL...

ConformU(1.3.0.15153)Setup.exe

Best wishes, Peter

RReverser commented 1 year ago

Hi Peter, I think that problem is gone but can't tell for sure because now running into another one: for some reason "Check Alpaca Protocol" hangs at the "Getting ImageArray..." step and never recovers, both on my camera and on the Simulator Camera and both in 1.2 and in 1.3 versions of ConformU:

image

This is particularly weird because in the logs I see that it received ImageArray successfully, e.g. this is from testing against the simulator:

...
PUT StopExposure              OK     ClientTransactionID is a string - Received HTTP status 400 (BadRequest) as expected.
 "A data validation error occurred for request /api/v1/camera/0/stopexposure with error message <The value 'qweqwe' is not valid.>. See https://datatracker.ietf.org/doc/html/rfc7231#section-6.5.1."
PUT StopExposure              OK       "A data validation error occurred for request /api/v1/camera/0/stopexposure with error message <The value 'qweqwe' is not valid.>. See https://datatracker.ietf.org/doc/html/rfc7231#section-6.5.1."
GET ImageArray                OK     Good ClientID and ClientTransactionID casing - The expected ClientTransactionID was returned: 67890
GET ImageArray                OK     Good ClientID and ClientTransactionID casing - Received HTTP status 200 (OK) as expected.
 {"Type":2,"Rank":3,"Value":[[[16,16,16],[16,16,16],[16,16,16],[16,16,16],[16,16,16],[16,16,16],[16,16,16],[16,16,16],...,]]],"ClientTransactionID":67890,"ServerTransactionID":377,"ErrorNumber":0,"ErrorMessage":"","DriverException":null}
(...hangs here...)
Peter-Simpson commented 1 year ago

That's odd, I can't reproduce this here. How large an image do you have configured? I'm using 600 x 400. Large dimension values can very quickly lead to very long processing times when using JSON. ImageBytes is much faster.

Do you have the image transfer type set to Best Available or ImageBytes?

Best wishes, Peter

image

RReverser commented 1 year ago

Do you have the image transfer type set to Best Available or ImageBytes?

I do have it set to Best Available (I believe, the default), I assume it would choose ImageBytes if available anyway?

I actually retested now and found that it seems to be working fine again, even though I haven't touched any settings since yesterday, and yesterday I tried resetting settings to default, waiting up to ~30 min for ConformU to finish those tests, etc. and nothing helped.

Oh well, at least it works again now, both with the simulator and with the actual camera. Apologies for the false alarm.

The casing tests do work as expected now, so closing this issue. Thanks again.

Peter-Simpson commented 1 year ago

Great, thanks fo retting me know.

Best wishes, Peter

RReverser commented 1 year ago

Looks like I have one follow up question: I see couple of failures like this

Issue Summary
PUT Connected ==> Ids Empty - Expected HTTP status : 400 (BadRequest) but received status: 200 (OK).
  Response: {"ClientTransactionID":67890,"ServerTransactionID":20,"ErrorNumber":1025,"ErrorMessage":"Invalid value for parameter \"Connected\": invalid value: string \"\", expected 'true' or 'false' in any casing"}

PUT Connected ==> Ids numeric - Expected HTTP status : 400 (BadRequest) but received status: 200 (OK).
  Response: {"ClientTransactionID":67890,"ServerTransactionID":21,"ErrorNumber":1025,"ErrorMessage":"Invalid value for parameter \"Connected\": invalid value: string \"123456\", expected 'true' or 'false' in any casing"}

PUT Connected ==> Ids string - Expected HTTP status : 400 (BadRequest) but received status: 200 (OK).
  Response: {"ClientTransactionID":67890,"ServerTransactionID":22,"ErrorNumber":1025,"ErrorMessage":"Invalid value for parameter \"Connected\": invalid value: string \"123456\", expected 'true' or 'false' in any casing"}

Why does it expect 400 Bad Request when passing non-boolean values to Connected parameter? As far as I can tell, for all other params when type doesn't match, tests expect 200 OK but with ASCOM "invalid value" error code which is what I'm returning for consistency.

Peter-Simpson commented 1 year ago

Hi Ingvar,

First off I need to let you know that parts of the error message text you quoted are misleading. The text "Ids empty", "Ids numeric" and "Ids string" should read "Bad parameter value - Empty string", "Bad parameter value - Number" and "Bad parameter value - Meaningless string". I'll fix this in the next release.

InvalidValueException is intended for use when the device understands exactly what is being asked of it but refuses to carry out the operation because the request value is outside the allowed range etc.

400 errors are expected when the device does not understand exactly what is being asked of it.

In the cases above the form parameter name is correctly cased but parameter's value cannot be reliably parsed to the expected bool type because the parameter value is neither "true" nor "false". Although the parameter name is correct, the device does not know what value the client is asking it to set, hence ConformU's expectation that the device will return a 400 error to show that it did not understand what was being asked of it.

Best wishes, Peter

RReverser commented 1 year ago

In the cases above the form parameter name is correctly cased but parameter's value cannot be reliably parsed to the expected bool type because the parameter value is neither "true" nor "false".

Hm I see. The line seems a bit blurry. Are you saying Invalid Value basically makes sense only for types like enums or device indices that have a separate type representation but further limited by range?

It just feels a bit odd because type-wise boolean is pretty much the same as enum - like enum might have allowed string values "0", "1", "2" that need to be mapped to corresponding enum variants, so does boolean have "true" and "false" (in arbitrary casing) that need to be mapped to literal boolean values correspondingly.

It feels natural to treat values outside the allowed set in the same way and return the same error, whether it's Bad Request or Invalid Value.

I guess what you're saying is that enum would need to be parsed to a number first, and then matched... But that's a bit of unfortunate complication. Right now I'm just using generic deserialization library that basically maps any string to given type T and returns the same error type if string didn't conform to the type, where type can be a string, a number, an enum or a boolean.

Trying to distinguish why a particular string didn't map to the given type would require more work yet...

RReverser commented 1 year ago

I wonder if ConformU could allow both kinds of errors for enums and booleans to accommodate parsers that don't want to do two-stage parsing and match strings to values directly... Would this be too much of deviation?

Peter-Simpson commented 1 year ago

Hi Ingvar,

I wonder if ConformU could allow both kinds of errors...

I think this is a good idea because some libraries and developers may have differing views on how to report parameter value errors.

I've updated ConformU to accept both 400 status and InvalidValue errors as valid error reports when parameter values cannot be parsed to the expected types: ConformU Installer.

While doing this I spotted that I had not included tests to send invalid values for mandatory parameters, so I have also added these. The new bad parameter value tests expect either a 400 status response or a 200 status JSON response that reports an InvalidValue error.

type-wise boolean is pretty much the same as enum

The point of my original post is that, to me, there is a difference between a device receiving a parameter value: that:

  1. Can be parsed to the expected type, but whose value the device considers inappropriate. E.g. specifying a telescope site elevation lower than -300 meters, which is the ASCOM specification minimum allowable site elevation. In this case an InvalidValue error is appropriate because the value is invalid from an ASCOM perspective.
  2. Is corrupt and can not be parsed to the expected type. In this case the device does not know what value the client intended to set, hence it can not say whether or not the value is valid in an ASCOM sense. In this case a 400 error sees appropriate to me.

Nonetheless, ConformU now accepts both 400 status and InvalidValue approaches.

Please let me know how you get on with the new version.

Best wishes, Peter

RReverser commented 1 year ago
  1. Can be parsed to the expected type, but whose value the device considers inappropriate. E.g. specifying a telescope site elevation lower than -300 meters, which is the ASCOM specification minimum allowable site elevation. In this case an InvalidValue error is appropriate because the value is invalid from an ASCOM perspective.

Yeah I agree this is unambiguously InvalidValue.

  1. Is corrupt and can not be parsed to the expected type. In this case the device does not know what value the client intended to set, hence it can not say whether or not the value is valid in an ASCOM sense. In this case a 400 error sees appropriate to me.

This makes sense. For values like strings, numbers and perhaps even booleans I think expecting only 400 bad request is perfectly reasonable - maybe worth leaving only the original Bad Request expectation there in place if that's the intended error type. I'm happy to update my code to use Bad Request for all of those if so.

The challenge arises only with enums I think, where some parsers might read it as a number first and then convert to an enum, so they can distinguish between "Bad Request: couldn't even parse to a number" / "Invalid Value: number out of range of allowed values" while other parsers might map string directly to enum variants so they need to choose a single error type, and for those I think it would be reasonable if ConformU expected either of the errors.

RReverser commented 1 year ago

Meanwhile I changed my parsing errors from InvalidValue to BadRequest as per above.