#5088 closed task (complete)

Create HttpResponse and HttpResponseCreator classes in libkea-http

Reported by: marcin Owned by: marcin
Priority: medium Milestone: Kea1.2
Component: remote-management Version: git
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DHCP Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description

The following section of the Control API design http://kea.isc.org/wiki/ControlAPIDesign#ProposedArchitecture calls for creating the HttpResponse and HttpResponseCreator classes to handle creation of the responses to HTTP requests. This ticket is to implement these classes.

Subtickets

Change History (18)

comment:1 Changed 11 months ago by marcin

  • Owner set to marcin
  • Status changed from new to accepted

comment:2 Changed 11 months ago by marcin

  • Owner changed from marcin to UnAssigned
  • Status changed from accepted to reviewing

I created a bunch of new classes within libkea-http for generating HTTP responses and hooking up to the libkea-http to provide custom response generation.

This comes with no changelog entry until libkea-http is fully functional.

comment:3 Changed 11 months ago by fdupont

  • Owner changed from UnAssigned to fdupont

comment:4 Changed 11 months ago by fdupont

I don't believe the ; alone in a catch body is useful when there is a block (or with other words you can either put a block, or a colon. Only the comment is needed).

comment:5 Changed 11 months ago by fdupont

I have a concern about locale in data_time code: on a system where the global locale is set to an unexpected value (e.g. Polish) it won't work at all. Note I can't test on one of my boxes because I never use a French locale (nor a French keyboard).

comment:6 Changed 11 months ago by fdupont

To finish about locale's: I recommend we add a warning comment:

  • the code relies on the fact locale's are not twisted, i.e., the code is not and must stay not internationalized.
  • in doubt reset the locale to its default std::locale::classic

comment:7 follow-up: Changed 11 months ago by fdupont

Why not using a POD for HttpVersion?? (please don't change the code, just explain the choice of a pair vs named fields)

comment:8 follow-up: Changed 11 months ago by fdupont

  • Owner changed from fdupont to marcin

According to RFC 2616 NO_CONTENT is 204 (vs 203), etc. So should we use the official status codes (IMHO we must)?

You use cbegin and cend so they should be checked in configure.ac (I can do it and merge #5092 en passant).

The HttpResponseCreator must be documented as being abstract so nobody will complain its creator can't be called directly.

I propose too to make createHttpResponse final (note in this case it should be made virtual too and the final keyword must be checked in configure).

The std::ostringstream s is not used in the HttpResponseJson::setBodyAsJson implementation.

If you'd like to be more fun you can try in date & time unit tests the 30 June 2015 at 23:59:60 which is the last leap second according to Google...

The HttpResponseCreatorTest::badRequest final check is not readable: I fixed it.

I updated configure.ac to check new C++11 feature and in a last commit I merged #5092 into it too.

comment:9 in reply to: ↑ 7 ; follow-up: Changed 11 months ago by marcin

Replying to fdupont:

Why not using a POD for HttpVersion?? (please don't change the code, just explain the choice of a pair vs named fields)

HTTP version is a pair of values and std::pair simply represents a pair of values and it looked like a good fit, rather than creating a new being.

comment:10 in reply to: ↑ 8 ; follow-up: Changed 11 months ago by marcin

Replying to fdupont:

According to RFC 2616 NO_CONTENT is 204 (vs 203), etc. So should we use the official status codes (IMHO we must)?

Good catch. Corrected!

You use cbegin and cend so they should be checked in configure.ac (I can do it and merge #5092 en passant).

Thanks.

The HttpResponseCreator must be documented as being abstract so nobody will complain its creator can't be called directly.

I updated the class description to say it is abstract.

I propose too to make createHttpResponse final (note in this case it should be made virtual too and the final keyword must be checked in configure).

I like that! Applied the change.

The std::ostringstream s is not used in the HttpResponseJson::setBodyAsJson implementation.

Removed unused variable.

If you'd like to be more fun you can try in date & time unit tests the 30 June 2015 at 23:59:60 which is the last leap second according to Google...

I didn't create this test, but I tried it locally and it seems that 31 Dec 2016 23:59:60 UTC is converted to 01 Jan 2017 00:00:00 UTC. Not sure this is what we'd expect.

The HttpResponseCreatorTest::badRequest final check is not readable: I fixed it.

I updated configure.ac to check new C++11 feature and in a last commit I merged #5092 into it too.

Thanks.

comment:11 Changed 11 months ago by marcin

  • Owner changed from marcin to fdupont

Finally, I now set std::locale::classic to the stream used for converting date/time. This seems to work fine - I forced global locale to "pl_PL" in the test prior to making this change and after making this change and in first case the test failed (as you'd expect) and in the second case it passed. I later removed the global locale setting because the available locales are system specific so I can't reliably force it to something other than classic.

comment:12 in reply to: ↑ 10 Changed 11 months ago by fdupont

Replying to marcin:

Replying to fdupont:

According to RFC 2616 NO_CONTENT is 204 (vs 203), etc. So should we use the official status codes (IMHO we must)?

Good catch. Corrected!

=> I am not sure it was the only incorrect value. As it seems we want to use the official codes I am look at them: done. I added a reference to the (obsolete) RFC 2068 and fixed a spelling error.

If you'd like to be more fun you can try in date & time unit tests the 30 June 2015 at 23:59:60 which is the last leap second according to Google...

I didn't create this test, but I tried it locally and it seems that 31 Dec 2016 23:59:60 UTC is converted to 01 Jan 2017 00:00:00 UTC. Not sure this is what we'd expect.

=> it is not correct so not we expect but at least it doesn't crash...

2 points remain: locale stuff but you improved the code so IMHO now there is no risk to get a problem from this, and the http version: for it I'll comment the answer.

comment:13 in reply to: ↑ 9 ; follow-up: Changed 11 months ago by fdupont

Replying to marcin:

Replying to fdupont:

Why not using a POD for HttpVersion?? (please don't change the code, just explain the choice of a pair vs named fields)

HTTP version is a pair of values and std::pair simply represents a pair of values and it looked like a good fit, rather than creating a new being.

=> hum, I don't buy this answer: C++ pair is not a builtin/primitive things: it is just a template around a first + second struct with an unreliable API (we had to fix code using pairs at least twice).

So now I really think it should be better to have a HttpVersion class with major and minor members: it is not so complex and for sure it is more natural/readable.

comment:14 Changed 11 months ago by fdupont

  • Owner changed from fdupont to marcin

comment:15 in reply to: ↑ 13 Changed 11 months ago by marcin

  • Owner changed from marcin to fdupont

Replying to fdupont:

Replying to marcin:

Replying to fdupont:

Why not using a POD for HttpVersion?? (please don't change the code, just explain the choice of a pair vs named fields)

HTTP version is a pair of values and std::pair simply represents a pair of values and it looked like a good fit, rather than creating a new being.

=> hum, I don't buy this answer: C++ pair is not a builtin/primitive things: it is just a template around a first + second struct with an unreliable API (we had to fix code using pairs at least twice).

So now I really think it should be better to have a HttpVersion class with major and minor members: it is not so complex and for sure it is more natural/readable.

Ok. I made this change.

comment:16 Changed 11 months ago by fdupont

Last commit seems OK. I launched the fastest Jenkins VM on it...

comment:17 Changed 11 months ago by fdupont

  • Owner changed from fdupont to marcin

Ready for merging!

comment:18 Changed 11 months ago by marcin

  • Resolution set to complete
  • Status changed from reviewing to closed

Merged with commit 715d18f961801ffbd798a65b19459178c3a53857

Note: See TracTickets for help on using tickets.