Details
Sent valid format of UUID and got true.
Sent wrong format of UUID and got false.
Diff Detail
- Repository
- rCOMM Comm
- Branch
- @max/@karol/tunnelbroker-newstack
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
services/tunnelbroker/docker-server/contents/server/src/Constants.h | ||
---|---|---|
21 | Is there any risk of multiple threads accessing this std::regex at the same time via std::regex_match? If so, is that behavior thread-safe? |
services/tunnelbroker/docker-server/contents/server/src/Constants.h | ||
---|---|---|
22 | Do we need to use word boundaries \b here? Do they improve anything? What do you think about reducing the duplication in this regex? Do you think it is more readable, or should we keep the current approach? | |
services/tunnelbroker/docker-server/contents/server/src/Tools/Tools.cpp | ||
66 | What are the exceptions that we expect here? |
services/tunnelbroker/docker-server/contents/server/src/Constants.h | ||
---|---|---|
21 |
Constant regexp is thread-safe, we don't have mutations here. | |
22 |
We can omit using \b here. I've removed them. | |
services/tunnelbroker/docker-server/contents/server/src/Tools/Tools.cpp | ||
66 |
I think we can catch regex_error here. |
services/tunnelbroker/docker-server/contents/server/src/Service/TunnelbrokerServiceImpl.cpp | ||
---|---|---|
153–159 ↗ | (On Diff #9424) | Why cannot we throw in validateSessionID and let the current catch handle the rest? |
195–201 ↗ | (On Diff #9424) | Why cannot we throw in validateSessionID and let the current catch handle the rest? |
services/tunnelbroker/docker-server/contents/server/src/Tools/Tools.cpp | ||
53–56 ↗ | (On Diff #9424) | Why cannot we just throw here? |
Can you rebase and retrigger the build by running arc diff so that the build appears green before landing?
Also curious about the question @karol-bisztyga asked, but I guess if the error is more specific that could be worth it.
services/tunnelbroker/docker-server/contents/server/src/Service/TunnelbrokerServiceImpl.cpp | ||
---|---|---|
153–159 ↗ | (On Diff #9424) |
If we throw an error here we can't log here and can return only one type of status code for grpc (which is described in catch). I'm sure we must think about logging the errors at the server-side and returning the detailed description to the client, because in case of error even if don't have the server logs we can rely on the user's bug report with the error and description that he received. In the app full-cycle, it works like: client app received an error and suggests the user fill the bug report or even send it automatically if the user agreed on this permission. That's why we need to send to the client detailed errors, to catch and resolve the possible bugs in the future. |
services/tunnelbroker/docker-server/contents/server/src/Tools/Tools.cpp | ||
53–56 ↗ | (On Diff #9424) |
It's not a good practice to throw an error inside the shared function and catch that error at the "upper" caller level. It will hard to debug this way and use this function properly. |