ibpsa / project1-boptest

Building Optimization Performance Tests
Other
104 stars 69 forks source link

BOPTEST examples sending Content-Type as application/x-www-form-urlencoded data #528

Closed kbenne closed 1 year ago

kbenne commented 1 year ago

When making requests to BOPTEST using the python requests library (as many of our examples do), using the data attribute sends the content as application/x-www-form-urlencoded data. I think this is fine for simple key value pairs, but the new results and forecast APIs have more complex json payloads. I think we get away with this most of the time, but I also think it is technically incorrect.

I think the examples should use the python requests json attribute instead as discussed here https://stackoverflow.com/questions/47188244/what-is-the-difference-between-the-data-and-json-named-arguments-with-reques

This will cause the content type to be application/json which is I think what we want.

Why am I bringing this up?

With the new results and forecast APIs, over in BOPTEST-Service the NodeJS express library seems to be handling the url encoded form data a bit differently from python flask. I find that when a request is made to either of these APIs with a point_names list of a single value, then the list is received and parsed as a single string value, not an array. I'm doing a workaround to catch this scenario. https://github.com/NREL/boptest-service/blob/77a55fac64b2a736eeb8cbbb12bad2c0a81eb1d8/web/server/src/routes/boptestRoutes.js#L401

This workaround is not required when sending the proper content-type header of application/json.

The nitty gritty about how express handles url encoded form data is documented a bit more here https://github.com/expressjs/body-parser#extended, where explains how an attempt is made to coherce form data into json.

@dhblum

javiarrobas commented 1 year ago

Just noting that this relates to the upgrade to Python 3 (see https://github.com/ibpsa/project1-boptest/issues/146) because the new Flask version requires specifying a certain location to parse for (see here). I realized this when working on a parallel project where I changed the Dockerfile to work with newest versions of pyfmi and Flask which forced me to change the API to use "json=..." instead of "data=...". See associated edits here. @dhblum mentioned to me that has added https://github.com/ibpsa/project1-boptest/issues/146 to Milestone v0.5.0.

dhblum commented 1 year ago

Thanks @kbenne for reporting this. @JavierArroyoBastida is right that this issue will need to be addressed also for #146. I think we could update examples to use json attribute instead of data attribute separately (and therefore sooner) than #146 to at least get it in master. The change might be pretty easy and I can make a PR shortly.

dhblum commented 1 year ago

Closed by https://github.com/ibpsa/project1-boptest/pull/531.