Implement HTTP server REST API #182

tribal-tec · 2016-10-19T10:01:43Z

added support for /schema and /registry endpoints
added ZeroEQHTTP target, moved from ZeroEQ library
rename subscribe() and register_() to handlePUT/GET accordingly
group RFCs in doxygen

eile · 2016-10-19T12:11:38Z

zeroeq/http/server.cpp

@@ -26,6 +32,30 @@ std::string _toLower( std::string value )
    return value;
 }

+// http://stackoverflow.com/questions/5343190/how-do-i-replace-all-instances-of-a-string-with-another-string


Tip: you can remove the /how-.... part

eile · 2016-10-19T12:12:25Z

zeroeq/http/server.cpp

+    {
+         subject.replace( pos, search.length(), replace );
+         pos += replace.length();
+    }


can also be written as a for loop.

To gain what?

Because it is one. Precondition is before while, loop statement is at the end of the while scope. Not important, though.

Not my implementation, hence the reference of origin. Worked at first shot, didn't take a second glance to make it even better.

eile · 2016-10-19T12:13:34Z

zeroeq/http/server.cpp

-    GETFuncMap _registrations;
+    typedef std::map< std::string, std::string > SchemaMap;
+    PUTFuncMap _PUT;
+    GETFuncMap _GET;


why not lower-case variable names?

looks more weird to me, but if you prefer

eile · 2016-10-19T12:16:05Z

tests/http/server.cpp

+
+    running = false;
+    thread.join();
+}


Missing tests:

Object named Schema: 'GET /foo/schema', 'GET /foo/schema/schema'

Object named Registry: throw when registering without namespace, 'GET /foo/registry', 'GET /foo/registry/schema'

should have been mentioned in the specification phase, now it's too late :)
Still not getting what exactly to test... there is already a test for garbage input, this is just structured garbage.

all GETs mentioned should return meaningful values, i.e., should not fail. I don't see in the spec that using an object named schema is forbidden, and I don't see a reason why it should be. idem for registry, except in the global namespace.

object named schema is tested, registry can be implemented, although I miss the use-case. global namespace registry is implemented, probably we're talking about different things here.

rdumusc

Looks good! However I thought we had decided to keep only a single remove() function and not the multitude of overloads of removePUT() and removeGET()? Somehow it does not figure in the spec, but I still don't see them as being especially useful.

rdumusc · 2016-10-19T13:41:57Z

zeroeq/http/server.cpp

+        event = _toLower( _replaceAll( event, "/", "::" ));
+        if( _put.count( event ) == 0 )
+            _schemas.erase( event );
+        return _get.erase(  event ) != 0;


style opinion # 2: I don't feel that the != 0 adds anything

It's just correct code. As long as there are compilers whining about int-to-bool conversion, I'm not changing that style.

rdumusc · 2016-10-19T13:43:16Z

zeroeq/http/server.cpp

-        event = _toLower( event );
-        if( _registrations.count( event ) != 0 )
+        event = _toLower( _replaceAll( event, "/", "::" ));
+        if( _get.count( event ) != 0 )


general style opinion: I find if( !_put.count( event )) more readable

oops went to wrong place, should have been 3 lines below. Here it would be if( _get.count( event )) of course

I don't know if it's relevant, but MSVC used to whine about a potential performance bug in the implicit int to bool conversion if you do that.

rdumusc · 2016-10-19T13:48:30Z

zeroeq/http/server.cpp

-            const size_t pos = name.find( '/' );
-            if( pos == std::string::npos )
-                return name;
+            const std::string key = _replaceAll( i.first, "::", "/" );


wouldn't it be more logical to store the keys with '/' directly? Only the servus::Serializable need a conversion at insertion time, no?

To me we could use '/' everywhere, also from the ZeroBuf code generator. I'm trying to be as less invasive as possible and not changing things w/o discussing them.

hernando · 2016-10-19T14:23:16Z

zeroeq/http/server.cpp

+    if( uri.length() < REQUEST_SCHEMA.length( ))
+        return false;
+    return uri.compare( uri.length() - REQUEST_SCHEMA.length(),
+                        REQUEST_SCHEMA.length(), REQUEST_SCHEMA ) == 0;


second parameter can be std::npos

uri.compare( uri.length() - REQUEST_SCHEMA.length(), std::npos, REQUEST_SCHEMA )

hernando · 2016-10-19T14:23:24Z

zeroeq/http/server.cpp

+}
+
+const std::string REQUEST_REGISTRY = "registry";
+const std::string REQUEST_SCHEMA = "schema";


No / before schema and registry?

The slash is stripped by _getTypeName(), so yes, no slash here.

hernando · 2016-10-19T14:35:55Z

zeroeq/http/server.h

-    ZEROEQ_API explicit Server();
-    ZEROEQ_API explicit Server( Server& shared )
+    ZEROEQ_API Server();
+    explicit Server( Server& shared )


ZEROEQ_API removed on purpose? If this function is internal tag it.

for inline functions we typically have no FOO_API declaration.

I always forget about this.

hernando · 2016-10-19T14:44:52Z

zeroeq/http/server.cpp

        }
+
+        return body.toStyledString();


If you have included the Jsoncpp source just to format the output of this response it's a bit of overkill in my opinion.

hernando · 2016-10-19T14:49:10Z

zeroeq/http/server.cpp

-        _subscriptions[ event ] = func;
+        _put[ event ] = func;
+        if( !schema.empty( ))
+            _schemas[ event ] = schema;


Throw if there's an schema and it's not the same (maybe a good reason to use jsoncpp, so you can compare schemas without failing due to whitespace)

You assume then that the schema is always JSON-formatted.

At least you should do a string comparison then. What does it mean to register GET and PUT with different schemas otherwise?

hernando · 2016-10-19T14:50:04Z

tests/http/server.cpp

-    BOOST_CHECK( server.add( foo ));
-    BOOST_CHECK( !server.add( foo ));
+    BOOST_CHECK( server.handle( foo ));
+    BOOST_CHECK( !server.handle( foo ));
    BOOST_CHECK( server.remove( foo ));
    BOOST_CHECK( !server.remove( foo ));


Add tests to verify that the schema is not removed if you remove the PUT handler but a GET handler stays.

hernando · 2016-10-20T13:47:43Z

zeroeq/http/server.cpp

+            const std::string& exist = _returnSchema( event );
+            if( !exist.empty() && schema != exist )
+                ZEROEQTHROW( std::runtime_error(
+                             "Schema registered for event differs: " + event ));
            _schemas[ event ] = schema;


Do no reassign if already the same.

eile

.

tribal-tec · 2016-10-21T11:28:16Z

Good now?

rdumusc · 2016-10-21T12:33:37Z

+1 for me despite my question about the removePUT/GETs has been ignored. I feel that we should have waited until (if?) we had a concrete need for so many methods, but as you wish :-)

tribal-tec · 2016-10-21T13:43:28Z

The remove... methods are only for symmetry reasons I guess. None of our code removes events from the server, so not even a remove() is needed. I can remove (haha) them actually.

* added support for /schema and /registry endpoints * added ZeroEQHTTP target, moved from ZeroEQ library * rename subscribe() and register_() to handlePUT/GET accordingly * group RFCs in doxygen

tribal-tec assigned eile, hernando, rdumusc and favreau Oct 19, 2016

eile previously requested changes Oct 19, 2016

View reviewed changes

rdumusc reviewed Oct 19, 2016

View reviewed changes

hernando reviewed Oct 19, 2016

View reviewed changes

hernando reviewed Oct 20, 2016

View reviewed changes

eile reviewed Oct 21, 2016

View reviewed changes

eile approved these changes Oct 21, 2016

View reviewed changes

Implement HTTP server REST API

2c30054

* added support for /schema and /registry endpoints * added ZeroEQHTTP target, moved from ZeroEQ library * rename subscribe() and register_() to handlePUT/GET accordingly * group RFCs in doxygen

tribal-tec merged commit 1019277 into HBPVIS:master Oct 21, 2016

Implement HTTP server REST API #182

Implement HTTP server REST API #182

Conversation

tribal-tec commented Oct 19, 2016

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

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

tribal-tec Oct 19, 2016 • edited Loading

Choose a reason for hiding this comment

rdumusc left a 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

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

hernando Oct 19, 2016 • edited Loading

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

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

eile left a comment

Choose a reason for hiding this comment

tribal-tec commented Oct 21, 2016

rdumusc commented Oct 21, 2016

tribal-tec commented Oct 21, 2016

tribal-tec Oct 19, 2016 •

edited

Loading

hernando Oct 19, 2016 •

edited

Loading