Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make sure to check post vars too when a post request is done to the API #223

Closed
wants to merge 5 commits into from

Conversation

dbursem
Copy link

@dbursem dbursem commented Dec 15, 2023

made this to address #220

instead of doing the if (request->hasParam()) {request->getParam()} routine everywhere, I decided to make a method that checks both the post and get parameters, returns them if set or throws otherwise.

Still a draft because I haven't finished testing and still want to refactor a bit (e.g. throwing more specific exception).

I'm not very fluent in C++ (my native language is PHP) so LMK if there's anything I can learn to improve.

@dingo35
Copy link

dingo35 commented Dec 15, 2023

I like your decision to make a separate method which tests for the parameters, it makes the changes on the existing code minimal, which I like!

Speaking of minimalizing changes, would it be possible to do the try/catch stuff in the new method, instead of it spreading around all the code?

BTW you have a typo in there FullSoc vs FullSoC

@dbursem
Copy link
Author

dbursem commented Dec 16, 2023

I like your decision to make a separate method which tests for the parameters, it makes the changes on the existing code minimal, which I like!

Speaking of minimalizing changes, would it be possible to do the try/catch stuff in the new method, instead of it spreading around all the code?

that would defeat the purpose. I use the exception to indicate the new method was not able to fetch the desired parameter.

Technically I could leave the if/else routines in tact by making replacements for both hasParam and getParam, but since the analogy for getParam will have to do both anyway (since it needs to determine if its a GET or POST param first), it made more sense to me to just use the one method and use an exception to indicate if the param is not found.

edit: If there's other ways to achieve the same I'd like to learn them.

I could however undo some of the refactoring I did, to keep the changes more to the point.

BTW you have a typo in there FullSoc vs FullSoC

doh, should've compiled first sorry about that.

@dingo35
Copy link

dingo35 commented Dec 18, 2023

Ok few remarks on your code here:

  1. update your git (git pull), you are not working on the current master
  2. your code does not seem to work. The "old" syntax still works:
    curl -X POST http://10.0.0.76/settings?override_current=150

but the "new¨, "normal" syntax doesn't:
curl -H "Content-Type: application/json" -d "override_current":"150" http://10.0.0.76/settings
curl -H "Content-Type: application/json" -d '{"override_current":"150"}' http://10.0.0.76/settings

  1. Also I understand the "try/catch" statement is pretty expensive, in case of an exception; it saves the entire stack in case it has to roll back. And the general consensus is an exception as to "out of memory" is acceptable, an exception as to "bad user input" is not.
    The arduino is a pretty small processor, and we are constantly battling to reduce both CPU and memory usage.
    So I would like to avoid the try/catch construct....

@dbursem
Copy link
Author

dbursem commented Dec 20, 2023

Thnx for the feedback! I have found a solution without the exception handling.

I did not test with a json content-type, I guess that's not supported by the AsyncWebServer library (at least not out of the box).
It does however work when using the default application/x-www-form-urlencoded:

➜  SmartEVSE-3 git:(check-post-vars-on-api-calls) curl -v -d 'L1=11' -d 'L2=12' -d 'L3=13' -d 'battery_current=5' http://192.168.1.225/currents
*   Trying 192.168.1.225:80...
* Connected to 192.168.1.225 (192.168.1.225) port 80 (#0)
> POST /currents HTTP/1.1
> Host: 192.168.1.225
> User-Agent: curl/7.81.0
> Accept: */*
> Content-Length: 35
> Content-Type: application/x-www-form-urlencoded
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Content-Length: 93
< Content-Type: application/json
< Connection: close
< Accept-Ranges: none
< 
* Closing connection 0
{"battery_current":5,"original":{"L":11,"":12,"SMART":13},"L":11,"":12,"SMART":13,"TOTAL":36}

Edit: This also makes a lot of JS in the frontend obsolete (can be replaced with regular HTML form submits) but I didn't want to go there (yet).

Not sure if I should also update the OpenAPI spec.

edit2: crap I think I messed up merging master, I think I'll close this MR and apply changes on a fresh branch (not going to bother rebasing/force-pushing)

@dbursem
Copy link
Author

dbursem commented Dec 20, 2023

continued work in #229

@dbursem dbursem closed this Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants