From 06cc6d742b41ef8116dad9bd1f7aa1b7efcdf219 Mon Sep 17 00:00:00 2001
From: Alex Peats-Bond <alexpb.tgm@gmail.com>
Date: Fri, 11 May 2018 13:51:15 -0700
Subject: [PATCH] Prevent fetch calls on IDL files

This change prevents users from calling `idl fetch` on a thrift
file. Users should be calling `fetch` with a service name instead.

A reported issue discovered that `update` wasn't fetching the latest
IDL for a service. Investigation showed that `meta.json` had incorrect
entries: some `remotes` specified a `.thrift` extension. This
indicates users calling `idl fetch` on a specifc IDL file. Reading the
`meta.json` file on later calls to `fetch` and `update` caused IDL to
silently fail with a 0 exit code.

Users would see IDL updates up to the first invalid entry and then IDL
would quit.

This change will update entries with `.thrift` extensions for
backwards compatibility.
---
 CHANGELOG.md |  5 +++--
 bin/idl.js   | 51 ++++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 41 insertions(+), 15 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 3169341..e928927 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,8 +1,9 @@
 
 # (unreleased)
 
-- No changes yet.
-
+- Prevent users from using `fetch` on an IDL file, instead of a service name.
+  This fixes a silent failure that would prevent updates after reading the IDL
+  entry in `meta.json`.
 
 # v3.1.12 (2018-01-25)
 
diff --git a/bin/idl.js b/bin/idl.js
index 8e2ad0f..cd3a18f 100755
--- a/bin/idl.js
+++ b/bin/idl.js
@@ -68,7 +68,8 @@ var envPrefixes = [
 
 var UnknownServiceError = TypedError({
     type: 'unknown-service',
-    message: 'The service {service} is not published in the registry',
+    message: 'The service {service} is not published in the registry.\n'+
+             'Ensure that you provide a service name and NOT the name of a specific IDL.',
     service: null
 });
 
@@ -498,15 +499,23 @@ function fetch(service, cb) {
 
 // fetchOneService is a utility to fetch or update a single service, used by
 // both fetch and update for updating individual services.
-function fetchOneService(service, cb) {
+//
+// 'remote' may not be equal to the service name.
+//
+// eg
+//  remote: foo/bar/baz.thrift
+//  service: foo/bar
+//
+// Either option is valid to prevent breaking older meta.json files.
+function fetchOneService(remote, cb) {
     // Precondition: "origin/master" is fetched and checked out in the IDL
     // registry cache.
     var self = this;
 
-    if (self.fetching.indexOf(service) >= 0) {
+    if (self.fetching.indexOf(remote) >= 0) {
         return cb(null);
     }
-    self.fetching.push(service);
+    self.fetching.push(remote);
 
     // Read $PWD/idl/meta.json
     var localMeta = MetaFile({
@@ -519,17 +528,23 @@ function fetchOneService(service, cb) {
 
     localMeta.readFile(onReadLocalMeta);
 
+    // 'service' will be set within onReadLocalMeta, but defined here
+    // to prevent passing the service name through a series of callbacks.
+    var service;
+
     function onReadLocalMeta(err) {
         if (err) {
             return cb(err);
         }
 
-        var alreadyFetched = findService(localMeta.toJSON(), service);
-        var existsInRegistry = findService(self.meta.toJSON(), service);
+        // Try to find the service name that corresponds to the remote, in
+        // the global meta.json file.
+        service = findServiceName(self.meta.toJSON(), remote);
 
-        if (!existsInRegistry) {
+        // check if the service exists in the global meta
+        if (!service) {
             cb(UnknownServiceError({
-                service: service
+                service: remote
             }));
         }
 
@@ -553,12 +568,22 @@ function fetchOneService(service, cb) {
         }, onCopied);
     }
 
-    function findService(json, service) {
-        var path = service.split('/');
+    // findServiceName takes in a 'remote' and returns the corresponding
+    // service name. This behaviour is required for backwards compatability.
+    //
+    // ie
+    //  remote: foo/bar/baz.thrift
+    //  service: foo/bar
+    //
+    // Returns the service name.
+    function findServiceName(meta, remote) {
+        var path = remote.split('/');
         for (var i = path.length; i >= 0; i--) {
-            var fetched = json.remotes[path.slice(0, i).join('/')];
-            if (fetched) {
-                return true;
+            var service = path.slice(0, i).join('/');
+
+            // var fetched = meta.remotes[service];
+            if (service in meta.remotes) {
+                return service;
             }
         }
         return false;