Skip to content

Add extra log when status code is not defined. #434

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

Conversation

konradkar2
Copy link
Contributor

@konradkar2 konradkar2 commented May 8, 2022

This refers to #430

Just to document behaviour when returning a status that is not well-known.
This issue could be fixed in a better way in the future, because for know it is only observed when using actual TCP connection -
simple UT that uses app.handle(req, res) will return user defined status code.

Added some extra check on UT that caused segfault on my pc.

@crow-clang-format
Copy link

--- include/crow/http_connection.h	(before formatting)
+++ include/crow/http_connection.h	(after formatting)
@@ -318,7 +318,9 @@
 
             if (!statusCodes.count(res.code))
             {
-                CROW_LOG_WARNING << this << " status code " << "(" << res.code << ")" << " not defined, returning 500 instead";
+                CROW_LOG_WARNING << this << " status code "
+                                 << "(" << res.code << ")"
+                                 << " not defined, returning 500 instead";
                 res.code = 500;
             }
 
--- tests/unittest.cpp	(before formatting)
+++ tests/unittest.cpp	(after formatting)
@@ -577,7 +577,6 @@
     SimpleApp app;
     CROW_ROUTE(app, "/get123")
     ([] {
-
         //this status does not exists statusCodes map defined in include/crow/http_connection.h
         const int undefinedStatusCode = 123;
         return response(undefinedStatusCode, "this should return 500");
@@ -592,8 +591,7 @@
     app.wait_for_server_start();
 
     asio::io_service is;
-    auto sendRequestAndGetStatusCode = [&is](const std::string & route) -> unsigned
-    {
+    auto sendRequestAndGetStatusCode = [&is](const std::string& route) -> unsigned {
         asio::ip::tcp::socket socket(is);
         socket.connect(asio::ip::tcp::endpoint(asio::ip::address::from_string("127.0.0.1"), 45451));
 

Copy link
Member

@The-EDev The-EDev left a comment

Choose a reason for hiding this comment

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

Aside from the comments and code formatting, it would be interesting to have some form of compile time check and warning for an unknown status code. Do you know if this is possible?

@konradkar2
Copy link
Contributor Author

Aside from the comments and code formatting, it would be interesting to have some form of compile time check and warning for an unknown status code. Do you know if this is possible?

Well I think this is a bigger change, what do you think about merging this one, and then the proper implementation could be added as different feature (reverting this one I guess).

@The-EDev
Copy link
Member

The-EDev commented May 8, 2022

Well I think this is a bigger change, what do you think about merging this one, and then the proper implementation could be added as different feature (reverting this one I guess).

Yeah I agree with you, I'm just wondering if it was possible, since I couldn't find much info on doing it online.

@konradkar2 konradkar2 force-pushed the logMessageWhenReturningNotWellKnownStatusCode branch from 661a973 to 6b7e121 Compare May 10, 2022 21:09
@konradkar2
Copy link
Contributor Author

Well I think this is a bigger change, what do you think about merging this one, and then the proper implementation could be added as different feature (reverting this one I guess).

Yeah I agree with you, I'm just wondering if it was possible, since I couldn't find much info on doing it online.

Well the problem with compile time check is that if we do it in crow::response then it will become hard to create response in runtime, or even make it impossible.

@konradkar2
Copy link
Contributor Author

konradkar2 commented May 10, 2022

Btw. why are we using "45451" as a port number in UTs? I've checked and port(0) results in random port, which is more robust I guess.

@The-EDev
Copy link
Member

It's more of a tradition, getting the app's port and setting the port to 0 were recent additions to crow (post v0.3). so the tests needed a port that was most likely not used and is predictable.

@konradkar2 konradkar2 requested a review from The-EDev May 11, 2022 17:19
Copy link
Member

@The-EDev The-EDev left a comment

Choose a reason for hiding this comment

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

Please replace REQUIRE with CHECK and an if statement in the unit tests, as a failing REQUIRE would prevent other tests from running.

Also CI seems to be exceeding the time limit (seems like some sort of blocking function / infinite loop)

konradkar2 and others added 2 commits May 18, 2022 15:57
Just to document behaviour when returning status that is
not well-known.
Using port 0 seems to cause the test to be blocked
Also added methods and status codes to Crow's documentation
@The-EDev The-EDev force-pushed the logMessageWhenReturningNotWellKnownStatusCode branch from 37e8bb7 to 564b070 Compare May 18, 2022 12:57
@The-EDev The-EDev linked an issue May 18, 2022 that may be closed by this pull request
@The-EDev The-EDev merged commit 0a75a57 into CrowCpp:master May 18, 2022
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.

Custom status codes handling
2 participants