verify that data is valid before generating journeys#142
Conversation
|
need your help for testing! |
kaligrafy
force-pushed
the
addExceptionWithInvalidDataInWhileLoops
branch
from
9de27d5 to
18946c7
Compare
| @@ -321,6 +321,8 @@ namespace TrRouting | |||
| NO_SERVICE_FROM_ORIGIN, | |||
| // There is no service to destination with the query parameters | |||
| NO_SERVICE_TO_DESTINATION | |||
kaligrafy
force-pushed
the
addExceptionWithInvalidDataInWhileLoops
branch
from
18946c7 to
5b97052
Compare
kaligrafy
force-pushed
the
addExceptionWithInvalidDataInWhileLoops
branch
3 times, most recently
from
2235d48 to
110df3b
Compare
|
Why changes in the m4 files? They make the builds fail on linux |
|
@greenscientist I could not run reconf or ./configure without auto changing the associated files in the m4 directory. Can you explain what is happening here? I removed the files for now, but I would like to know why they were changed. |
kaligrafy
force-pushed
the
addExceptionWithInvalidDataInWhileLoops
branch
from
110df3b to
1fb8564
Compare
|
When I run autoreconf -i on macos, it outputs this and change the two files: |
tahini
left a comment
tahini
left a comment
There was a problem hiding this comment.
I still have bad data lying around, I'll test this on monday.
| odTripsActivities.clear(); | ||
| odTripsModes.clear(); | ||
|
|
||
| accessNodesIdx.clear(); |
There was a problem hiding this comment.
I think you merged with the last commit of v2c... Please rebase on v2c to remove this
kaligrafy
force-pushed
the
addExceptionWithInvalidDataInWhileLoops
branch
from
1fb8564 to
5012bac
Compare
the two while loops in forward and reverse journey can create memory leak (infinite push into a vector) when invalid data is found (like reverse travel times or incorrectly rounded/floored/ceiled travel time seconds that was imported as float, see #141, needs testing though to close issue
kaligrafy
force-pushed
the
addExceptionWithInvalidDataInWhileLoops
branch
from
5012bac to
3e050be
Compare
tahini
left a comment
tahini
left a comment
There was a problem hiding this comment.
It works with faulty data. Just make sure it is properly handled, and it's the right type of exception that is thrown (see comments)
| NO_SERVICE_TO_DESTINATION | ||
| NO_SERVICE_TO_DESTINATION, | ||
| // Invalid data found, which could create memory leaks or incorrect results | ||
| INVALID_DATA |
There was a problem hiding this comment.
You are adding a new reason for no routing, so you should also add it
- in result_to_v1.cpp, in the noRoutingFoundResponse function, it should return an appropriate string message.
- In the API.yml file, in the reasons for no routing (search for NO_SERVICE_TO_DESTINATION)
- Is it possible to think of faulty data that could cause this problem to happen? If so, you could add a unit test for this reason (one that would fail with current branch and suceed now). If it is possible to at least describe the kind of data in a dummy unit test in csa_route_calculation_test.cpp, then someone could add a test later
| { | ||
| public: | ||
| InvalidDataException(NoRoutingReason reason_) : std::exception(), reason(reason_) {}; | ||
| NoRoutingReason getReason() const { return reason; }; |
There was a problem hiding this comment.
Nvm my preivous comment depending on the answer here: This is a new exception type, that will be caught by the catch-all clause in transit_routing_http_server and return a "Wrong or malformed query". In that case
- It's not really the query who's malformed, it's the data! The returned value will be counterintuitive to the caller
- It's technically not a NoRoutingReason. You could throw the NoRoutingFoundException with the INVALID_DATA reason, or if you want a new exception, you should put a new enum for those reasons, add it to the API and return a better error message to the user in those cases.
|
Leaving it here for @kaligrafy for max 6 months and then will close. |
3 participants
the two while loops in forward and reverse journey can create memory leak (infinite push into a vector) when invalid data is found (like reverse travel times or incorrectly rounded/floored/ceiled travel time seconds that was imported as float, see #141, needs testing though to close issue