Delete feed from RDBMS #134

landonreed · 2018-08-28T16:02:33Z

This PR adds the ability to delete a feed (essentially dropping a schema) to the RDBMS-backed code path.

It also updates the documentation for the cmd line interface (in addition to adding the --delete and --json options).

Finally, it tweaks the TableLoadResult and FeedLoadResult objects to include fields for fileSize and filename, respectively.

This also updates the documentation of the command line features.

…leting record) This is a breaking change in that it requires any existing SQL database for gtfs-lib to add the feeds#deleted boolean column. BREAKING CHANGE

codecov-io · 2018-08-28T17:55:17Z

Codecov Report

Merging #134 into dev will decrease coverage by 0.23%.
The diff coverage is 22.58%.

@@             Coverage Diff              @@
##                dev     #134      +/-   ##
============================================
- Coverage     48.62%   48.38%   -0.24%     
- Complexity      595      596       +1     
============================================
  Files           142      142              
  Lines          6974     7023      +49     
  Branches        809      817       +8     
============================================
+ Hits           3391     3398       +7     
- Misses         3279     3320      +41     
- Partials        304      305       +1

Impacted Files	Coverage Δ	Complexity Δ
...java/com/conveyal/gtfs/loader/TableLoadResult.java	`100% <ø> (ø)`	`1 <0> (ø)`	⬇️
...onveyal/gtfs/validator/PatternFinderValidator.java	`85.36% <ø> (ø)`	`9 <0> (ø)`	⬇️
.../java/com/conveyal/gtfs/loader/FeedLoadResult.java	`100% <ø> (ø)`	`2 <0> (ø)`	⬇️
...in/java/com/conveyal/gtfs/graphql/GTFSGraphQL.java	`66.66% <100%> (+4.16%)`	`3 <0> (ø)`	⬇️
src/main/java/com/conveyal/gtfs/GTFS.java	`40.11% <14.54%> (-12.3%)`	`9 <2> (ø)`
.../java/com/conveyal/gtfs/loader/JdbcGtfsLoader.java	`63.11% <80%> (+0.38%)`	`20 <1> (+1)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4069ccd...75200e9. Read the comment docs.

v4.0.0

abyrd · 2018-09-11T16:06:25Z

src/main/java/com/conveyal/gtfs/GTFS.java

            printHelp(options);
            return;
        }
-
+        boolean storeResults = cmd.hasOption("json");
+        File directory = null;


I would move the creation of the objectMapper down here, conditional upon storeResults=true.

abyrd · 2018-09-11T16:06:50Z

src/main/java/com/conveyal/gtfs/GTFS.java


+            if (namespaceToDelete != null) {
+                LOG.info("Exporting feed with unique identifier {}", namespaceToDelete);


Message appears to be incorrect.

abyrd · 2018-09-11T16:08:55Z

src/main/java/com/conveyal/gtfs/GTFS.java

@@ -252,7 +324,7 @@ private static Options getOptions () {
                .desc("zip file path for the exported GTFS").build());
        options.addOption(Option.builder().longOpt("load").hasArg()
                .argName("file").desc("load GTFS data from the given file").build());
-        options.addOption(Option.builder().longOpt("validate").hasArg().optionalArg(true).argName("feed")
+        options.addOption(Option.builder().longOpt("validate").hasArg().optionalArg(true).argName("feedId")


"feedId" has another meaning distinct from our unique-IDs for loaded feed versions ("namespaces"). We should probably avoid use of the word feedId.

I would agree with that. However, it has been used in a number of places within this class already to mean namespace often with a parenthetical clarifying that it is referring to the schema identifier. This would actually be a breaking change -- so could we just clarify the meaning in the cmd line option description?

I don't necessarily think that we should change this right away, now that I see how many times the word feedId is used in this class. But I don't see why it would be a breaking change. The validate tasks's argument is called feed rather than feedId so the command line options are already a bit incoherent. But I don't even think the argument names are used anywhere. We'd want to also change some parameter names, but since parameters are positional in Java it wouldn't break callers.

I will make a ticket for this if we're not going to change it soon.

Oh, you're right. Changing the name feedId wouldn't affect any command line instructions, just the output of the help text. I will update these arg names here and we can use the ticket you create to track down/update other inconsistent usages.

abyrd

It looks like there were no changes to the use of the term "feedId", was this an intentional decision to continue using the term? Is it used in this sense elsewhere in the user-facing usage messages?

abyrd · 2018-09-12T04:07:25Z

src/main/java/com/conveyal/gtfs/GTFS.java

@@ -179,8 +179,8 @@ public static DataSource createDataSource (String url, String username, String p
     * It also lets you run a GraphQL API for all the feeds loaded into the database.
     */
    public static void main (String[] args) throws IOException {
-        // Object mapper used for writing load or validation results to file.
-        ObjectMapper mapper = new ObjectMapper();
+        // Object mapper used for writing load or validation results to file (instantia).


There appears to be stray text in this comment. This is a detail, but I'd find the code easier to read if the variable were declared and initialized to null just before it is conditionally set to a new instance, around line 211 where we also declare File directory = null;

landonreed · 2018-09-12T20:36:27Z

@abyrd, I hadn't changed anything related to the use of feedId because I was awaiting a response to the above comment: #134 (comment).

abyrd · 2018-09-13T14:41:47Z

Oh, didn't see that comment before. I just replied. We can always do it later, maybe just make an issue to clean this up later.

landonreed · 2018-09-17T20:11:37Z

I'm going to go ahead and merge this. I've created a ticket to track the namespace/feedId conflation issue here: #142.

landonreed · 2018-11-26T17:52:40Z

🎉 This PR is included in version 4.1.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

landonreed added 6 commits August 28, 2018 11:54

feat(delete): add delete feed feature to SQL code path

94323ae

refactor(cmd-line): add delete and json to command line options

c8a77f8

This also updates the documentation of the command line features.

refactor(graphql): use non-deprecated method for building GraphQL object

94a3ae4

refactor(load): add filename field to FeedLoadResult

67e0763

refactor(load): add fileSize field to TableLoadResult

7acbc6d

style(comments): fix/add comments

0eedbdd

landonreed requested a review from abyrd August 28, 2018 16:02

refactor(delete): mark feed as deleted in feeds table (rather than de…

44038e9

…leting record) This is a breaking change in that it requires any existing SQL database for gtfs-lib to add the feeds#deleted boolean column. BREAKING CHANGE

Landon Reed and others added 6 commits August 29, 2018 14:11

Merge pull request #136 from conveyal/dev

e87ab5f

v4.0.0

Merge pull request #137 from conveyal/dev

75e54cf

v4.0.0

Merge pull request #139 from conveyal/dev

32f17ca

v4.0.0

4.0.0 [ci skip]

1b394e0

Prepare next development iteration 4.0.1-SNAPSHOT [ci skip]

3328c4b

Merge branch 'master' into delete-feed

811e999

abyrd requested changes Sep 11, 2018

View reviewed changes

landonreed added 2 commits September 11, 2018 15:38

refactor: address PR #134 comments

a346fca

refactor: add comment

75200e9

abyrd requested changes Sep 12, 2018

View reviewed changes

move object mapper closer to instantiation

8c9d605

clean up whitespace for options, feedId -> namespace

c08ba7f

landonreed merged commit f98abe5 into dev Sep 17, 2018

landonreed deleted the delete-feed branch September 17, 2018 20:11

landonreed added the released label Nov 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delete feed from RDBMS #134

Delete feed from RDBMS #134

landonreed commented Aug 28, 2018

codecov-io commented Aug 28, 2018 •

edited

Loading

abyrd Sep 11, 2018

abyrd Sep 11, 2018

abyrd Sep 11, 2018

landonreed Sep 11, 2018 •

edited

Loading

abyrd Sep 13, 2018

landonreed Sep 13, 2018

abyrd left a comment

abyrd Sep 12, 2018

landonreed commented Sep 12, 2018

abyrd commented Sep 13, 2018

landonreed commented Sep 17, 2018

landonreed commented Nov 26, 2018


		if (namespaceToDelete != null) {
		LOG.info("Exporting feed with unique identifier {}", namespaceToDelete);

Delete feed from RDBMS #134

Delete feed from RDBMS #134

Conversation

landonreed commented Aug 28, 2018

codecov-io commented Aug 28, 2018 • edited Loading

Codecov Report

abyrd Sep 11, 2018

Choose a reason for hiding this comment

abyrd Sep 11, 2018

Choose a reason for hiding this comment

abyrd Sep 11, 2018

Choose a reason for hiding this comment

landonreed Sep 11, 2018 • edited Loading

Choose a reason for hiding this comment

abyrd Sep 13, 2018

Choose a reason for hiding this comment

landonreed Sep 13, 2018

Choose a reason for hiding this comment

abyrd left a comment

Choose a reason for hiding this comment

abyrd Sep 12, 2018

Choose a reason for hiding this comment

landonreed commented Sep 12, 2018

abyrd commented Sep 13, 2018

landonreed commented Sep 17, 2018

landonreed commented Nov 26, 2018

codecov-io commented Aug 28, 2018 •

edited

Loading

landonreed Sep 11, 2018 •

edited

Loading