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

Fix action POST #7

Merged
merged 17 commits into from
Aug 23, 2021
Merged

Fix action POST #7

merged 17 commits into from
Aug 23, 2021

Conversation

Citrullin
Copy link

Fixes POST on action. Makes it possible to POST the correct json object defined in the TD at the defined action endpoint.

delete newBuffer;
return;
request->send(404);
return;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you expand why now are we returning a 404 where before this was a 500?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. That's wrong. I wanted to find the ThingActionObject first and returned a 404 when it wasn't able to find it. Was a bit confused. Good catch. Fixed it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok cool, don't we need to clear the memory as it was done previously?

Also, previously we had an if statement just above this check, looking for the presence of the action id. In my understanding, this is removed due to the fact that it was something specific from the Withings protocol. Now we comply with the data payload described in the TD. right?

Copy link
Author

@Citrullin Citrullin Aug 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I understand the function is only called when the URL already exist. So, the 404 should return before that. Just checked it. As it turns out, this doesn't happen. But this check has to happen somewhere else. I will take a look into it. It should send a 404.
In the previous implementation it checked, if the json object contains the action id, for some reason. Guess that has something to do with the Mozilla standard. That is different with the w3c standard. So, not necessary anymore.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, it crashed before, because there was a generic handler for any /action/* endpoint, even if you didn't had a listener for it. I guess the Mozilla standard handled it different and returned an object with all available actions or something like that. I just removed it. You get the desired 404 now, if the action doesn't exist. Makes the code more simple as well.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the clean memory part. That's a good point. Not necessary, but good to do.
Yes, it now follows the TD payload structure and the webthings specific stuff is removed. Even though you can technically also define booleans etc. as input, but that cashes the code. Just added it as issue in #12

@relu91 relu91 merged commit f4596c7 into relu91:dev Aug 23, 2021
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.

3 participants