Skip to content
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.

Sign up for GitHub

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

Jump to bottom

verify that data is valid before generating journeys #142

Open
wants to merge 1 commit into
base: v2c
Choose a base branch
from
Open

verify that data is valid before generating journeys #142

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 19 additions & 10 deletions connection_scan_algorithm/src/forward_journey.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "agency.hpp"
#include "routing_result.hpp"
#include "toolbox.hpp" //MAX_INT
#include <iostream>
tahini marked this conversation as resolved.
Show resolved Hide resolved

namespace TrRouting
{
Expand Down Expand Up @@ -79,7 +80,7 @@ namespace TrRouting
int inVehicleDistance {-1}; int totalInVehicleDistance { 0}; int totalWalkingDistance { 0};
int totalTransferDistance {-1}; int accessDistance { 0}; int egressDistance { 0};
int journeyStepTravelTime {-1}; int accessWalkingTime {-1}; int bestAccessNodeIndex {-1};
int transferTime {-1}; int egressWalkingTime {-1};
int transferTime {-1}; int egressWalkingTime {-1}; int bestAccessNodePrevIndex{-1};
int waitingTime {-1}; int accessWaitingTime {-1};

for (auto & resultingNodeIndex : resultingNodes)
Expand All @@ -96,16 +97,16 @@ namespace TrRouting
modeShortnames.clear();
inVehicleTravelTimesSeconds.clear();

totalInVehicleTime = 0; transferArrivalTime = -1; firstDepartureTime = -1;
totalWalkingTime = 0; transferReadyTime = -1; minimizedDepartureTime = -1;
totalWaitingTime = 0; departureTime = -1; numberOfTransfers = -1;
totalTransferWalkingTime = 0; arrivalTime = -1; boardingSequence = -1;
totalTransferWaitingTime = 0; inVehicleTime = -1; unboardingSequence = -1;
totalInVehicleTime = 0; transferArrivalTime = -1; firstDepartureTime = -1;
totalWalkingTime = 0; transferReadyTime = -1; minimizedDepartureTime = -1;
totalWaitingTime = 0; departureTime = -1; numberOfTransfers = -1;
totalTransferWalkingTime = 0; arrivalTime = -1; boardingSequence = -1;
totalTransferWaitingTime = 0; inVehicleTime = -1; unboardingSequence = -1;
totalDistance = 0; distance = -1;
inVehicleDistance = 0; totalInVehicleDistance = 0; totalWalkingDistance = 0;
totalTransferDistance = 0; accessDistance = 0; egressDistance = 0;
journeyStepTravelTime = -1; accessWalkingTime = -1; bestAccessNodeIndex = -1;
transferTime = -1; egressWalkingTime = -1;
inVehicleDistance = 0; totalInVehicleDistance = 0; totalWalkingDistance = 0;
totalTransferDistance = 0; accessDistance = 0; egressDistance = 0;
journeyStepTravelTime = -1; accessWalkingTime = -1; bestAccessNodeIndex = -1;
transferTime = -1; egressWalkingTime = -1; bestAccessNodePrevIndex = -1;
waitingTime = -1; accessWaitingTime = -1;

//std::cerr << nodes[resultingNodeIndex].get()->name << " : " << std::get<0>(forwardJourneysSteps[resultingNodeIndex]) << std::endl;
Expand All @@ -117,11 +118,19 @@ namespace TrRouting
continue;
}


while ((std::get<journeyStepIndexes::FINAL_ENTER_CONNECTION>(resultingNodeJourneyStep) != -1 && std::get<journeyStepIndexes::FINAL_EXIT_CONNECTION>(resultingNodeJourneyStep) != -1))
{
journey.push_front(resultingNodeJourneyStep);
bestAccessNodeIndex = std::get<connectionIndexes::NODE_DEP>(*forwardConnections[std::get<journeyStepIndexes::FINAL_ENTER_CONNECTION>(resultingNodeJourneyStep)].get());
if (bestAccessNodePrevIndex != -1 && bestAccessNodeIndex != -1 && bestAccessNodePrevIndex == bestAccessNodeIndex) {
// this should not happen, it means that there is an ambiguity in the data, which makes this loop runs forever and create a memory leak filling journey indefinitely.
// TODO: add tests
std::cerr << "Invalid data in path uuid: " << paths[trips[std::get<journeyStepIndexes::FINAL_TRIP>(resultingNodeJourneyStep)].get()->pathIdx].get()->uuid << std::endl;
throw InvalidDataException(NoRoutingReason::INVALID_DATA);
}
resultingNodeJourneyStep = forwardJourneysSteps[bestAccessNodeIndex];
bestAccessNodePrevIndex = bestAccessNodeIndex;
}

if (!params.returnAllNodesResult)
Expand Down
40 changes: 24 additions & 16 deletions connection_scan_algorithm/src/reverse_journey.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
#include "path.hpp"
#include "agency.hpp"
#include "routing_result.hpp"
#include <iostream>
tahini marked this conversation as resolved.
Show resolved Hide resolved

namespace TrRouting
{
Expand Down Expand Up @@ -68,14 +69,14 @@ namespace TrRouting
Path * journeyStepPath;
Agency * journeyStepAgency;

int totalInVehicleTime { 0}; int transferArrivalTime {-1}; int firstDepartureTime {-1};
int totalWalkingTime { 0}; int transferReadyTime {-1}; int numberOfTransfers {-1};
int totalWaitingTime { 0}; int departureTime {-1}; int boardingSequence {-1};
int totalTransferWalkingTime { 0}; int arrivalTime {-1}; int unboardingSequence {-1};
int totalTransferWaitingTime { 0}; int inVehicleTime {-1}; int bestEgressNodeIndex {-1};
int totalDistance { 0}; int distance {-1};
int inVehicleDistance {-1}; int totalInVehicleDistance { 0}; int totalWalkingDistance { 0};
int totalTransferDistance {-1}; int accessDistance { 0}; int egressDistance { 0};
int totalInVehicleTime { 0}; int transferArrivalTime {-1}; int firstDepartureTime {-1};
int totalWalkingTime { 0}; int transferReadyTime {-1}; int numberOfTransfers {-1};
int totalWaitingTime { 0}; int departureTime {-1}; int boardingSequence {-1};
int totalTransferWalkingTime { 0}; int arrivalTime {-1}; int unboardingSequence {-1};
int totalTransferWaitingTime { 0}; int inVehicleTime {-1}; int bestEgressNodeIndex {-1};
int totalDistance { 0}; int distance {-1}; int bestEgressNodePrevIndex{-1};
int inVehicleDistance {-1}; int totalInVehicleDistance { 0}; int totalWalkingDistance { 0};
int totalTransferDistance {-1}; int accessDistance { 0}; int egressDistance { 0};
int journeyStepTravelTime {-1}; int accessWalkingTime {-1};
int transferTime {-1}; int egressWalkingTime {-1};
int waitingTime {-1}; int accessWaitingTime {-1};
Expand All @@ -94,14 +95,14 @@ namespace TrRouting
modeShortnames.clear();
inVehicleTravelTimesSeconds.clear();

totalInVehicleTime = 0; transferArrivalTime = -1; firstDepartureTime = -1;
totalWalkingTime = 0; transferReadyTime = -1; numberOfTransfers = -1;
totalWaitingTime = 0; departureTime = -1; boardingSequence = -1;
totalTransferWalkingTime = 0; arrivalTime = -1; unboardingSequence = -1;
totalTransferWaitingTime = 0; inVehicleTime = -1; bestEgressNodeIndex = -1;
totalDistance = 0; distance = -1;
inVehicleDistance = 0; totalInVehicleDistance = 0; totalWalkingDistance = 0;
totalTransferDistance = 0; accessDistance = 0; egressDistance = 0;
totalInVehicleTime = 0; transferArrivalTime = -1; firstDepartureTime = -1;
totalWalkingTime = 0; transferReadyTime = -1; numberOfTransfers = -1;
totalWaitingTime = 0; departureTime = -1; boardingSequence = -1;
totalTransferWalkingTime = 0; arrivalTime = -1; unboardingSequence = -1;
totalTransferWaitingTime = 0; inVehicleTime = -1; bestEgressNodeIndex = -1;
totalDistance = 0; distance = -1; bestEgressNodePrevIndex = -1;
inVehicleDistance = 0; totalInVehicleDistance = 0; totalWalkingDistance = 0;
totalTransferDistance = 0; accessDistance = 0; egressDistance = 0;
journeyStepTravelTime = -1; accessWalkingTime = -1;
transferTime = -1; egressWalkingTime = -1;
waitingTime = -1; accessWaitingTime = -1;
Expand All @@ -124,7 +125,14 @@ namespace TrRouting
}
journey.push_back(resultingNodeJourneyStep);
bestEgressNodeIndex = std::get<connectionIndexes::NODE_ARR>(*reverseConnections[std::get<journeyStepIndexes::FINAL_EXIT_CONNECTION>(resultingNodeJourneyStep)].get());
if (bestEgressNodePrevIndex != -1 && bestEgressNodeIndex != -1 && bestEgressNodePrevIndex == bestEgressNodeIndex) {
// this should not happen, it means that there is an ambiguity in the data, which makes this loop runs forever and create a memory leak filling journey indefinitely.
// TODO: add tests
std::cerr << "Invalid data in path uuid: " << paths[trips[std::get<journeyStepIndexes::FINAL_TRIP>(resultingNodeJourneyStep)].get()->pathIdx].get()->uuid << std::endl;
throw InvalidDataException(NoRoutingReason::INVALID_DATA);
}
resultingNodeJourneyStep = reverseJourneysSteps[bestEgressNodeIndex];
bestEgressNodePrevIndex = bestEgressNodeIndex;
}

if (!params.returnAllNodesResult)
Expand Down
18 changes: 17 additions & 1 deletion include/routing_result.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,7 +320,9 @@ namespace TrRouting
// There is no service from origin with the query parameters
NO_SERVICE_FROM_ORIGIN,
// There is no service to destination with the query parameters
NO_SERVICE_TO_DESTINATION
NO_SERVICE_TO_DESTINATION,
// Invalid data found, which could create memory leaks or incorrect results
INVALID_DATA
Copy link
Collaborator

Choose a reason for hiding this comment

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

You are adding a new reason for no routing, so you should also add it

  1. in result_to_v1.cpp, in the noRoutingFoundResponse function, it should return an appropriate string message.
  2. In the API.yml file, in the reasons for no routing (search for NO_SERVICE_TO_DESTINATION)
  3. 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

};

/**
Expand All @@ -337,6 +339,20 @@ namespace TrRouting
NoRoutingReason reason;
};

/**
* @brief Exception class thrown when getting invalid data
*
*/
class InvalidDataException : public std::exception
{
public:
InvalidDataException(NoRoutingReason reason_) : std::exception(), reason(reason_) {};
NoRoutingReason getReason() const { return reason; };
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

  1. It's not really the query who's malformed, it's the data! The returned value will be counterintuitive to the caller
  2. 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.


private:
NoRoutingReason reason;
};

}

#endif // TR_ROUTING_RESULT