HttpRequest improvements and move HttpParams into separate module #1498

mikee47 · 2018-10-21T20:08:03Z

Rename HttpRequest::stream to bodyStream
Move trivial code into headers
Simplify getHeader() and getPostParameter() methods by calling HashMap[] const method, thus ensuring missing values aren't added. Also more efficient as doesn't require two lookups (one for contains(), another for operator[]).
Simplify and optimise HttpRequest::getBody() by reading content directly into pre-allocated String

WebHelpers/escape

bugfix: encoding of space as '+' added
uri_escape(const char*, int) rewritten without using heap
add uri_unescape_inplace(char*, int)
bugfix: output buffer must account for nul terminator in uri_unescape_inplace(String&)

structures.h

Move HttpParams definition into separate module

UrlencodedOutputStream

Remove (duplicate) definition of HttpParams
Move output code into HttpParams::printTo() method

* Move trivial code into headers * Simplify getHeader() and getPostParameter() methods by calling `HashMap[] const` method, thus ensuring missing values aren't added. Also more efficient as doesn't require two lookups (one for contains(), another for operator[]). * Simplify and optimise HttpRequest::getBody() by reading content directly into pre-allocated String

WebHelpers/escape * bugfix: encoding of space as '+' added * uri_escape(const char*, int) rewritten without using heap * add uri_unescape_inplace(char*, int) * bugfix: output buffer must account for nul terminator in uri_unescape_inplace(String&) structures.h * Move HttpParams definition into separate module UrlencodedOutputStream * Remove (duplicate) definition of HttpParams * Move output code into HttpParams::printTo() method

slaff · 2018-10-21T20:28:38Z

Sming/SmingCore/Network/Http/HttpRequest.h

 	HttpRequest* setBody(uint8_t* rawData, size_t length);

 	/**
 	 * @brief Instead of storing the response body we can set a stream that will take care to process it
 	 * @param ReadWriteStream *stream
 	 *
 	 * @retval HttpRequest*
+	 *
+	 * @note The response to this message will be stored in the user-provided stream.


The response to this message. Did you mean The response to this request ?

Yes! Well spotted.

slaff · 2018-10-21T20:32:02Z

Sming/SmingCore/Network/Http/HttpRequest.h


-	HttpRequest* setHeader(const String& name, const String& value);
+	/** @deprecated this replaces existing post parameter set, which is different to


Can you add here also a very small example of the new recommended way? That target audience is beginners.

setPostParameters() isn't actually used anywhere in Sming or its samples. Would it be safe to delete?

If we're keeping it, I'd probably amend the comment to something like:

To set all post parameters, use request.postParams = params To copy additional post parameters, use request.postParams.setMultiple(params)

If we're keeping it,

Yes, we will keep it for now. For deprecation we usually do the following:

once a method is marked as deprecated we remove its use inside of Sming and samples

we announce it as deprecated and suggest replacement (in code comments and/or git comments)

we create a new 3.x release with the deprecated method in it + information in release notes

we remove the deprecated method in following 3.x release (might be 3.x+1, but that is not always the case. If the impact is low then we can schedule the removal in the next 3.x+1 release.

slaff · 2018-10-21T21:56:43Z

@mikee47 Is it tested on real device with real app ?

mikee47 · 2018-10-22T08:30:41Z

@mikee47 Is it tested on real device with real app ?

Yes, the escaping changes are tested and verified. This PR is mainly refactoring, the substantive changes are yet to come.
I've attempted to get the Basic_WebSkeletonApp sample working as it exercises HttpResult::getBody() but it fails to work as-is. I've pushed a few changes to make it work.

slaff · 2018-10-22T09:27:46Z

Sming/SmingCore/Network/Http/HttpServerConnection.cpp

@@ -280,6 +280,8 @@ int HttpServerConnection::staticOnBody(http_parser* parser, const char* at, size
 	}

 	// TODO: ...
+	connection->request.setBody((const uint8_t *)&at, length);


That is something that we don't do. Because it is the easiest way to crash remotely the http server and the device. Just send a request with very long body. What we do is use body parsers and decide in there how to operate over the incoming response body.

That said, please, remove that new line.

slaff · 2018-10-22T09:28:29Z

Sming/SmingCore/Network/Http/HttpServerConnection.cpp

@@ -280,6 +280,8 @@ int HttpServerConnection::staticOnBody(http_parser* parser, const char* at, size
 	}

 	// TODO: ...
+	connection->request.setBody((const uint8_t *)&at, length);


That is something that we don't do. Because it is the easiest way to crash remotely the http server and the device. Just send a request with very long body. What we do is use body parsers and decide in there how to operate over the incoming response body.

That said, please, remove that new line.

slaff · 2018-10-22T09:28:51Z

Sming/SmingCore/Network/Http/HttpServerConnection.cpp

@@ -280,6 +280,8 @@ int HttpServerConnection::staticOnBody(http_parser* parser, const char* at, size
 	}

 	// TODO: ...
+	connection->request.setBody((const uint8_t *)&at, length);


That is something that we don't do. Because it is the easiest way to crash remotely the http server and the device. Just send a request with very long body. What we do is use body parsers and decide in there how to operate over the incoming response body.

That said, please, remove that new line.

slaff · 2018-10-22T15:44:26Z

Sming/SmingCore/Network/Http/HttpServerConnection.cpp

@@ -280,6 +280,8 @@ int HttpServerConnection::staticOnBody(http_parser* parser, const char* at, size
 	}

 	// TODO: ...
+	connection->request.setBody((const uint8_t*)&at, length);


connection->request.setBody((const uint8_t*)&at, length);

That is something that we don't do. Because it is the easiest way to crash remotely the http server and the device. Just send a request with very long body. What we do is use body parsers and decide in there how to operate over the incoming response body.

That said, please, remove that new line.

mikee47 · 2018-10-22T19:46:14Z

@slaff I've reverted everything to do with the Basic_WebSkeletonApp, bad example for this PR.

mikee47 · 2018-10-22T19:57:15Z

Is it tested on real device with real app ?

Tested with HttpServer_ConfigNetwork and works fine. Code is migrated from my dev. branch which it's tested on first. We're moving into trickier territory now, lots of dependencies.

slaff · 2018-10-22T20:06:36Z

Tested with HttpServer_ConfigNetwork and works fine. Code is migrated from my dev. branch which it's tested on first. We're moving into trickier territory now, lots of dependencies.

I guess that means that the PR is ready to be tested by me, right?

mikee47 · 2018-10-22T20:09:01Z

I guess that means that the PR is ready to be test by me, right?

Yes please, I've nothing else to add for this PR.

slaff · 2018-10-23T21:16:54Z

@mikee47 That PR broke my HTTPS client connections... My wild guess is that either some data is sent before the handshake is over... or I will have to update my code which means that we should add more information how to migrate to the new code.

Can you check it on your system? The HttpClient sample would be a good start.

slaff · 2018-10-23T21:24:42Z

Sming/SmingCore/Network/Http/HttpRequest.h

-	uint32_t getSslOptions();
+	HttpRequest* setSslOptions(uint32_t sslOptions)
+	{
+		sslOptions = sslOptions;


That looks like a wrong code to me? Shouldn't it be this->sslOptions = sslOptions; ?

Yep, it's wrong - my error - does it fix your problem?

Yep, that was the problem. Can you put a PR with the small fix? Sorry for not noticing this earlier. There should be a way to detect such problems automatically.

I made the error moving the code into the header - yes, it would be useful to be able to track those sorts of changes in diff. More recently I've been doing those changes (cut & paste) in an initial commit so shouldn't happen again.

@slaff Thank you!

The issue is fixed with #1504. No action is needed from your side.

yes, it would be useful to be able to track those sorts of changes in diff.

I guess my IDE, cpplint or codacy should be able to detect the issue. Will look tomorrow how to enable that in codacy by default.

mikee47 added 3 commits October 21, 2018 21:09

Rename HttpRequest::stream to bodyStream

bbde0d1

mikee47 force-pushed the feature/HttpParams branch from ca5d3bd to b4a1117 Compare October 21, 2018 20:09

slaff requested changes Oct 21, 2018

View reviewed changes

slaff added this to the 3.7.0 milestone Oct 21, 2018

slaff added the 3 - Review label Oct 21, 2018

Review changes (amend comments)

e8edfbd

slaff approved these changes Oct 21, 2018

View reviewed changes

mikee47 force-pushed the feature/HttpParams branch from 206c905 to d998e9a Compare October 22, 2018 10:20

slaff requested changes Oct 22, 2018

View reviewed changes

HttpParams needs virtual destructor

1a4decf

mikee47 force-pushed the feature/HttpParams branch from d998e9a to 1a4decf Compare October 22, 2018 19:39

mikee47 mentioned this pull request Oct 22, 2018

Notice: Recheck your PRs #1501

Closed

slaff removed the 3 - Review label Oct 22, 2018

slaff merged commit da53bdf into SmingHub:develop Oct 22, 2018

mikee47 deleted the feature/HttpParams branch October 22, 2018 21:38

slaff mentioned this pull request Oct 23, 2018

Preparation for Release 3.7.0 #1426

Closed

4 tasks

slaff reviewed Oct 23, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HttpRequest improvements and move HttpParams into separate module #1498

HttpRequest improvements and move HttpParams into separate module #1498

mikee47 commented Oct 21, 2018

slaff Oct 21, 2018

mikee47 Oct 21, 2018

slaff Oct 21, 2018

mikee47 Oct 21, 2018

mikee47 Oct 21, 2018

slaff Oct 21, 2018

slaff commented Oct 21, 2018

mikee47 commented Oct 22, 2018

slaff Oct 22, 2018

slaff Oct 22, 2018

slaff Oct 22, 2018

slaff Oct 22, 2018

mikee47 commented Oct 22, 2018

mikee47 commented Oct 22, 2018 •

edited by slaff

Loading

slaff commented Oct 22, 2018 •

edited

Loading

mikee47 commented Oct 22, 2018 •

edited

Loading

slaff commented Oct 23, 2018 •

edited

Loading

slaff Oct 23, 2018

mikee47 Oct 23, 2018

slaff Oct 23, 2018

mikee47 Oct 23, 2018 •

edited

Loading

mikee47 Oct 23, 2018

slaff Oct 23, 2018


		HttpRequest* setHeader(const String& name, const String& value);
		/** @deprecated this replaces existing post parameter set, which is different to

HttpRequest improvements and move HttpParams into separate module #1498

HttpRequest improvements and move HttpParams into separate module #1498

Conversation

mikee47 commented Oct 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slaff commented Oct 21, 2018

mikee47 commented Oct 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikee47 commented Oct 22, 2018

mikee47 commented Oct 22, 2018 • edited by slaff Loading

slaff commented Oct 22, 2018 • edited Loading

mikee47 commented Oct 22, 2018 • edited Loading

slaff commented Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikee47 Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikee47 commented Oct 22, 2018 •

edited by slaff

Loading

slaff commented Oct 22, 2018 •

edited

Loading

mikee47 commented Oct 22, 2018 •

edited

Loading

slaff commented Oct 23, 2018 •

edited

Loading

mikee47 Oct 23, 2018 •

edited

Loading