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

Editor: Allow null values for foreign ref fields or required fields where permitted #133

Merged
merged 11 commits into from
Nov 8, 2018
Merged

Editor: Allow null values for foreign ref fields or required fields where permitted #133

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
d63d0fb
fix(editor): allow null values for foreign ref fields allowed to be e…
landonreed Aug 23, 2018
8189831
Merge branch 'dev' into fix-fare-rule-no-route
landonreed Oct 15, 2018
40ef82d
fix(editor): permit empty values if required field allows it
landonreed Oct 16, 2018
5b87be8
fix(snapshot): check for null namespace in tableExists method
landonreed Oct 16, 2018
49b4611
test: tweak javadoc
landonreed Oct 16, 2018
305df47
test(editor): add basic CRUD test for fares
landonreed Oct 16, 2018
a6943aa
test: add FareDTO class
landonreed Oct 16, 2018
78a7785
test(editor): add simple checks for fare_rules
landonreed Oct 16, 2018
795fdcb
refactor(test): change FareDTO#transfer_duration to Integer
landonreed Oct 16, 2018
fbadc7b
test(editor): add CRUD routes test
landonreed Oct 16, 2018
d10aff3
test: address PR comments
landonreed Nov 8, 2018
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
11 changes: 8 additions & 3 deletions src/main/java/com/conveyal/gtfs/loader/JdbcGtfsSnapshotter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,8 +176,7 @@ private TableLoadResult copy (Table table, boolean createIndexes) {
*/
private TableLoadResult createScheduleExceptionsTable() {
// check to see if the schedule_exceptions table exists
boolean scheduleExceptionsTableExists = feedIdToSnapshot != null &&
tableExists(feedIdToSnapshot, "schedule_exceptions");
boolean scheduleExceptionsTableExists = tableExists(feedIdToSnapshot, "schedule_exceptions");
String scheduleExceptionsTableName = tablePrefix + "schedule_exceptions";

if (scheduleExceptionsTableExists) {
Expand Down Expand Up @@ -271,7 +270,11 @@ private TableLoadResult createScheduleExceptionsTable() {

// determine if we appear to be working with a calendar_dates-only feed.
// If so, we must also add dummy entries to the calendar table
if (!tableExists(feedIdToSnapshot, "calendar") && calendarDatesReader.getRowCount() > 0) {
if (
feedIdToSnapshot != null &&
!tableExists(feedIdToSnapshot, "calendar") &&
calendarDatesReader.getRowCount() > 0
) {
sql = String.format(
"insert into %s (service_id, description, start_date, end_date, " +
"monday, tuesday, wednesday, thursday, friday, saturday, sunday)" +
Expand Down Expand Up @@ -322,6 +325,8 @@ private TableLoadResult createScheduleExceptionsTable() {
* Helper method to determine if a table exists within a namespace.
*/
private boolean tableExists(String namespace, String tableName) {
// Preempt SQL check with null check of either namespace or table name.
if (namespace == null || tableName == null) return false;
try {
// This statement is postgres-specific.
PreparedStatement tableExistsStatement = connection.prepareStatement(
Expand Down
15 changes: 11 additions & 4 deletions src/main/java/com/conveyal/gtfs/loader/JdbcTableWriter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,10 +292,13 @@ private void setStatementParameters(ObjectNode jsonObject, Table table, Prepared
continue;
}
JsonNode value = jsonObject.get(field.name);
// LOG.info("{}={}", field.name, value);
LOG.debug("{}={}", field.name, value);
try {
if (value == null || value.isNull()) {
if (field.isRequired()) {
if (field.isRequired() && !field.isEmptyValuePermitted()) {
// Only register the field as missing if the value is null, the field is required, and empty
// values are not permitted. For example, a null value for fare_attributes#transfers should not
// trigger a missing field exception.
missingFieldNames.add(field.name);
continue;
}
Expand All @@ -318,7 +321,7 @@ private void setStatementParameters(ObjectNode jsonObject, Table table, Prepared
// FIXME: This is a hack to get arrival and departure time into the right format. Because the UI
// currently returns them as seconds since midnight rather than the Field-defined format HH:MM:SS.
try {
if (value == null ||value.isNull()) {
if (value == null || value.isNull()) {
if (field.isRequired()) {
missingFieldNames.add(field.name);
continue;
Expand Down Expand Up @@ -421,7 +424,11 @@ private void updateChildTable(ArrayNode subEntities, Integer id, boolean isCreat
// field statement above.
for (Field field : subTable.specFields()) {
if (field.referenceTable != null && !field.referenceTable.name.equals(specTable.name)) {
referencesPerTable.put(field.referenceTable, subEntity.get(field.name).asText());
JsonNode refValueNode = subEntity.get(field.name);
// Skip over references that are null but not required (e.g., route_id in fare_rules).
if (refValueNode.isNull() && !field.isRequired()) continue;
String refValue = refValueNode.asText();
referencesPerTable.put(field.referenceTable, refValue);
}
}
// Insert new sub-entity.
Expand Down
10 changes: 7 additions & 3 deletions src/test/java/com/conveyal/gtfs/GTFSTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
import static org.junit.Assert.assertThat;

/**
* A test suite for the GTFS Class
* A test suite for the {@link GTFS} Class.
*/
public class GTFSTest {
private final ByteArrayOutputStream outContent = new ByteArrayOutputStream();
Expand Down Expand Up @@ -570,7 +570,9 @@ private void assertThatPersistenceExpectationRecordWasFound(int numRecordsSearch
);
}

// a helper class to verify that data got stored in a particular table
/**
* A helper class to verify that data got stored in a particular table.
*/
private class PersistenceExpectation {
public String tableName;
// each persistence expectation has an array of record expectations which all must reference a single row
Expand All @@ -590,7 +592,9 @@ private enum ExpectedFieldType {
DOUBLE, STRING
}

// a helper class to verify that data got stored in a particular record
/**
* A helper class to verify that data got stored in a particular record.
*/
private class RecordExpectation {
public double acceptedDelta;
public double doubleExpectation;
Expand Down
198 changes: 183 additions & 15 deletions src/test/java/com/conveyal/gtfs/loader/JDBCTableWriterTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
package com.conveyal.gtfs.loader;

import com.conveyal.gtfs.TestUtils;
import com.conveyal.gtfs.util.FareDTO;
import com.conveyal.gtfs.util.FareRuleDTO;
import com.conveyal.gtfs.util.FeedInfoDTO;
import com.conveyal.gtfs.util.InvalidNamespaceException;
import com.conveyal.gtfs.util.RouteDTO;
import com.fasterxml.jackson.databind.ObjectMapper;
import org.junit.AfterClass;
import org.junit.BeforeClass;
Expand All @@ -28,14 +31,19 @@ public class JDBCTableWriterTest {
private static String testDBName;
private static DataSource testDataSource;
private static String testNamespace;
private static final ObjectMapper mapper = new ObjectMapper();

private static JdbcTableWriter createTestTableWriter (Table table) throws InvalidNamespaceException {
return new JdbcTableWriter(table, testDataSource, testNamespace);
}

@BeforeClass
public static void setUpClass() throws SQLException {
// create a new database
testDBName = TestUtils.generateNewDB();
String dbConnectionUrl = String.format("jdbc:postgresql://localhost/%s", testDBName);
testDataSource = createDataSource (dbConnectionUrl, null, null);
LOG.info("creating feeds table because it isn't automtically generated unless you import a feed");
LOG.info("creating feeds table because it isn't automatically generated unless you import a feed");
Connection connection = testDataSource.getConnection();
connection.createStatement()
.execute("create table if not exists feeds (namespace varchar primary key, md5 varchar, " +
Expand All @@ -50,8 +58,10 @@ public static void setUpClass() throws SQLException {
}

@Test
public void canCreateUpdateAndDeleteFeedinfoEntities () throws IOException, SQLException, InvalidNamespaceException {
ObjectMapper mapper = new ObjectMapper();
public void canCreateUpdateAndDeleteFeedInfoEntities() throws IOException, SQLException, InvalidNamespaceException {
// Store Table and Class values for use in test.
final Table feedInfoTable = Table.FEED_INFO;
final Class<FeedInfoDTO> feedInfoDTOClass = FeedInfoDTO.class;

// create new object to be saved
FeedInfoDTO feedInfoInput = new FeedInfoDTO();
Expand All @@ -63,13 +73,13 @@ public void canCreateUpdateAndDeleteFeedinfoEntities () throws IOException, SQLE
feedInfoInput.default_route_type = "3";

// convert object to json and save it
JdbcTableWriter createTableWriter = new JdbcTableWriter(Table.FEED_INFO, testDataSource, testNamespace);
JdbcTableWriter createTableWriter = createTestTableWriter(feedInfoTable);
String createOutput = createTableWriter.create(mapper.writeValueAsString(feedInfoInput), true);
LOG.info("create output:");
LOG.info("create {} output:", feedInfoTable.name);
LOG.info(createOutput);

// parse output
FeedInfoDTO createdFeedInfo = mapper.readValue(createOutput, FeedInfoDTO.class);
FeedInfoDTO createdFeedInfo = mapper.readValue(createOutput, feedInfoDTOClass);

// make sure saved data matches expected data
assertThat(createdFeedInfo.feed_publisher_name, equalTo(publisherName));
Expand All @@ -79,34 +89,34 @@ public void canCreateUpdateAndDeleteFeedinfoEntities () throws IOException, SQLE
createdFeedInfo.feed_publisher_name = updatedPublisherName;

// covert object to json and save it
JdbcTableWriter updateTableWriter = new JdbcTableWriter(Table.FEED_INFO, testDataSource, testNamespace);
JdbcTableWriter updateTableWriter = createTestTableWriter(feedInfoTable);
String updateOutput = updateTableWriter.update(
createdFeedInfo.id,
mapper.writeValueAsString(createdFeedInfo),
true
);
LOG.info("update output:");
LOG.info("update {} output:", feedInfoTable.name);
LOG.info(updateOutput);

FeedInfoDTO updatedFeedInfoDTO = mapper.readValue(updateOutput, FeedInfoDTO.class);
FeedInfoDTO updatedFeedInfoDTO = mapper.readValue(updateOutput, feedInfoDTOClass);

// make sure saved data matches expected data
assertThat(updatedFeedInfoDTO.feed_publisher_name, equalTo(updatedPublisherName));

// try to delete record
JdbcTableWriter deleteTableWriter = new JdbcTableWriter(Table.FEED_INFO, testDataSource, testNamespace);
JdbcTableWriter deleteTableWriter = createTestTableWriter(feedInfoTable);
int deleteOutput = deleteTableWriter.delete(
createdFeedInfo.id,
true
);
LOG.info("delete output:");
LOG.info("delete {} output:", feedInfoTable.name);
LOG.info(updateOutput);

// make sure record does not exist in DB
String sql = String.format(
"select * from %s.%s where id=%d",
testNamespace,
Table.FEED_INFO.name,
feedInfoTable.name,
createdFeedInfo.id
);
LOG.info(sql);
Expand All @@ -116,8 +126,6 @@ public void canCreateUpdateAndDeleteFeedinfoEntities () throws IOException, SQLE

@Test
public void canPreventSQLInjection() throws IOException, SQLException, InvalidNamespaceException {
ObjectMapper mapper = new ObjectMapper();

// create new object to be saved
FeedInfoDTO feedInfoInput = new FeedInfoDTO();
String publisherName = "' OR 1 = 1; SELECT '1";
Expand All @@ -128,7 +136,7 @@ public void canPreventSQLInjection() throws IOException, SQLException, InvalidNa
feedInfoInput.default_route_type = "3";

// convert object to json and save it
JdbcTableWriter createTableWriter = new JdbcTableWriter(Table.FEED_INFO, testDataSource, testNamespace);
JdbcTableWriter createTableWriter = createTestTableWriter(Table.FEED_INFO);
String createOutput = createTableWriter.create(mapper.writeValueAsString(feedInfoInput), true);
LOG.info("create output:");
LOG.info(createOutput);
Expand All @@ -140,6 +148,166 @@ public void canPreventSQLInjection() throws IOException, SQLException, InvalidNa
assertThat(createdFeedInfo.feed_publisher_name, equalTo(publisherName));
}

@Test
public void canCreateUpdateAndDeleteFares() throws IOException, SQLException, InvalidNamespaceException {
// Store Table and Class values for use in test.
final Table fareTable = Table.FARE_ATTRIBUTES;
final Class<FareDTO> fareDTOClass = FareDTO.class;

// create new object to be saved
FareDTO fareInput = new FareDTO();
String fareId = "2A";
fareInput.fare_id = fareId;
fareInput.currency_type = "USD";
fareInput.price = 2.50;
fareInput.agency_id = "RTA";
fareInput.payment_method = 0;
// Empty value should be permitted for transfers and transfer_duration
fareInput.transfers = null;
fareInput.transfer_duration = null;
FareRuleDTO fareRuleInput = new FareRuleDTO();
// Fare ID should be assigned to "child entity" by editor automatically.
fareRuleInput.fare_id = null;
fareRuleInput.route_id = null;
// FIXME There is currently no check for valid zone_id values in contains_id, origin_id, and destination_id.
fareRuleInput.contains_id = "any";
fareRuleInput.origin_id = "value";
fareRuleInput.destination_id = "permitted";
fareInput.fare_rules = new FareRuleDTO[]{fareRuleInput};

// convert object to json and save it
JdbcTableWriter createTableWriter = createTestTableWriter(fareTable);
String createOutput = createTableWriter.create(mapper.writeValueAsString(fareInput), true);
LOG.info("create {} output:", fareTable.name);
LOG.info(createOutput);

// parse output
FareDTO createdFare = mapper.readValue(createOutput, fareDTOClass);

// make sure saved data matches expected data
assertThat(createdFare.fare_id, equalTo(fareId));
assertThat(createdFare.fare_rules[0].fare_id, equalTo(fareId));

// try to update record
String updatedFareId = "3B";
createdFare.fare_id = updatedFareId;

// covert object to json and save it
JdbcTableWriter updateTableWriter = createTestTableWriter(fareTable);
String updateOutput = updateTableWriter.update(
createdFare.id,
mapper.writeValueAsString(createdFare),
true
);
LOG.info("update {} output:", fareTable.name);
LOG.info(updateOutput);

FareDTO updatedFareDTO = mapper.readValue(updateOutput, fareDTOClass);

// make sure saved data matches expected data
assertThat(updatedFareDTO.fare_id, equalTo(updatedFareId));
assertThat(updatedFareDTO.fare_rules[0].fare_id, equalTo(updatedFareId));

// try to delete record
JdbcTableWriter deleteTableWriter = createTestTableWriter(fareTable);
int deleteOutput = deleteTableWriter.delete(
createdFare.id,
true
);
LOG.info("delete {} output:", fareTable.name);
LOG.info(updateOutput);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be deleteOutput.


// make sure fare_attributes record does not exist in DB
String sql = String.format(
"select * from %s.%s where id=%d",
testNamespace,
fareTable.name,
createdFare.id
);
LOG.info(sql);
ResultSet rs = testDataSource.getConnection().prepareStatement(sql).executeQuery();
assertThat(rs.getFetchSize(), equalTo(0));

// make sure fare_rules record does not exist in DB
String fareRulesSql = String.format(
"select * from %s.%s where id=%d",
testNamespace,
Table.FARE_RULES.name,
createdFare.fare_rules[0].id
);
LOG.info(fareRulesSql);
ResultSet fareRulesResultSet = testDataSource.getConnection().prepareStatement(fareRulesSql).executeQuery();
assertThat(fareRulesResultSet.getFetchSize(), equalTo(0));
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that I look at these tests, it seems like they could really use a sql check to make sure that the data was created and updated. Also, these 2 lines could be made into a reusable method assertThatSqlQueryYieldsZeroRows or something like that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea.

Copy link
Contributor Author

@landonreed landonreed Nov 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, actually I think the checking for created/updated data gets a little muddied because we would then need to ensure that we're using the same connection to see the data we expect. I think the first part of your comment is out of scope, but I will add a TODO.

}

@Test
public void canCreateUpdateAndDeleteRoutes() throws IOException, SQLException, InvalidNamespaceException {
// Store Table and Class values for use in test.
final Table routeTable = Table.ROUTES;
final Class<RouteDTO> routeDTOClass = RouteDTO.class;

// create new object to be saved
RouteDTO routeInput = new RouteDTO();
String routeId = "500";
routeInput.route_id = routeId;
routeInput.agency_id = "RTA";
// Empty value should be permitted for transfers and transfer_duration
routeInput.route_short_name = "500";
routeInput.route_long_name = "Hollingsworth";
routeInput.route_type = 3;

// convert object to json and save it
JdbcTableWriter createTableWriter = createTestTableWriter(routeTable);
String createOutput = createTableWriter.create(mapper.writeValueAsString(routeInput), true);
LOG.info("create {} output:", routeTable.name);
LOG.info(createOutput);

// parse output
RouteDTO createdFare = mapper.readValue(createOutput, routeDTOClass);

// make sure saved data matches expected data
assertThat(createdFare.route_id, equalTo(routeId));

// try to update record
String updatedRouteId = "600";
createdFare.route_id = updatedRouteId;

// covert object to json and save it
JdbcTableWriter updateTableWriter = createTestTableWriter(routeTable);
String updateOutput = updateTableWriter.update(
createdFare.id,
mapper.writeValueAsString(createdFare),
true
);
LOG.info("update {} output:", routeTable.name);
LOG.info(updateOutput);

RouteDTO updatedRouteDTO = mapper.readValue(updateOutput, routeDTOClass);

// make sure saved data matches expected data
assertThat(updatedRouteDTO.route_id, equalTo(updatedRouteId));

// try to delete record
JdbcTableWriter deleteTableWriter = createTestTableWriter(routeTable);
int deleteOutput = deleteTableWriter.delete(
createdFare.id,
true
);
LOG.info("delete {} output:", routeTable.name);
LOG.info(updateOutput);

// make sure route record does not exist in DB
String sql = String.format(
"select * from %s.%s where id=%d",
testNamespace,
routeTable.name,
createdFare.id
);
LOG.info(sql);
ResultSet rs = testDataSource.getConnection().prepareStatement(sql).executeQuery();
assertThat(rs.getFetchSize(), equalTo(0));
}

@AfterClass
public static void tearDownClass() {
TestUtils.dropDB(testDBName);
Expand Down
Loading