FEATURE: Badge query validation, preview results, and EXPLAIN

Upon saving a badge or requesting a badge result preview,
BadgeGranter.contract_checks! will examine the provided badge SQL for
some contractual obligations - namely, the returned columns and use of
trigger parameters.

Saving the badge is wrapped in a transaction to make this easier, by
raising ActiveRecord::Rollback on a detected violation.

On the client, a modal view is added for the badge query sample run
results, named admin-badge-preview.
The preview action is moved up to the route.
The save action, on failure, triggers a 'saveError' action (also in the
route).

The preview action gains a new parameter, 'explain', which will give the
output of an EXPLAIN query for the badge sql, which can be used by forum
admins to estimate the cost of their badge queries.
The preview link is replaced by two links, one which omits (false) and
includes (true) the EXPLAIN query.

The Badge.save() method is amended to propogate errors.

Badge::Trigger gets some utility methods for use in the
BadgeGranter.contract_checks! method.

Additionally, extra checks outside of BadgeGranter.contract_checks! are
added in the preview() method, to cover cases of null granted_at
columns.

An uninitialized variable path is removed in the backfill() method.

TODO - it would be nice to be able to get the actual names of all
columns the provided query returns, so we could give more errors

app/assets/javascripts/admin/controllers/admin-badge-preview.js.es6

@@ -0,0 +1,49 @@
+export default Ember.Controller.extend({
+  needs: ['modal'],
+
+  sample: Em.computed.alias('model.sample'),
+  errors: Em.computed.alias('model.errors'),
+  count: Em.computed.alias('model.grant_count'),
+
+  count_warning: function() {
+    if (this.get('count') <= 10) {
+      return this.get('sample.length') !== this.get('count');
+    } else {
+      return this.get('sample.length') !== 10;
+    }
+  }.property('count', 'sample.length'),
+
+  has_query_plan: function() {
+    return !!this.get('model.query_plan');
+  }.property('model.query_plan'),
+
+  query_plan_html: function() {
+    var raw = this.get('model.query_plan'),
+        returned = "<pre>";
+
+    _.each(raw, function(linehash) {
+      returned += Handlebars.Utils.escapeExpression(linehash["QUERY PLAN"]);
+      returned += "<br>";
+    });
+
+    returned += "</pre>";
+    return returned;
+  }.property('model.query_plan'),
+
+  processed_sample: Ember.computed.map('model.sample', function(grant) {
+    var i18nKey = 'admin.badges.preview.grant.with',
+        i18nParams = { username: Handlebars.Utils.escapeExpression(grant.username) };
+
+    if (grant.post_id) {
+      i18nKey += "_post";
+      i18nParams.link = "<a href='/p/" + grant.post_id + "' data-auto-route='true'>" + Handlebars.Utils.escapeExpression(grant.title) + "</a>";
+    }
+
+    if (grant.granted_at) {
+      i18nKey += "_time";
+      i18nParams.time = Handlebars.Utils.escapeExpression(moment(grant.granted_at).format(I18n.t('dates.long_with_year')));
+    }
+
+    return I18n.t(i18nKey, i18nParams);
+  })
+});

app/assets/javascripts/admin/controllers/admin-badges.js.es6

@@ -78,21 +78,6 @@ export default Ember.ArrayController.extend({
 
   actions: {
 
-    preview: function(badge) {
-      // TODO wire modal and localize
-      Discourse.ajax('/admin/badges/preview.json', {
-        method: 'post',
-        data: {sql: badge.query, target_posts: !!badge.target_posts}
-      }).then(function(json){
-        if(json.error){
-          bootbox.alert(json.error);
-        } else {
-          bootbox.alert(json.grant_count + " badges to be assigned");
-        }
-      });
-
-    },
-
     /**
       Create a new badge and select it.
 
@@ -128,16 +113,21 @@ export default Ember.ArrayController.extend({
                      'enabled', 'show_posts',
                      'target_posts', 'name', 'description',
                      'icon', 'query', 'badge_grouping_id',
-                     'trigger', 'badge_type_id'];
+                     'trigger', 'badge_type_id'],
+            self = this;
 
-        if(this.get('selectedItem.system')){
+        if (this.get('selectedItem.system')){
           var protectedFields = this.get('protectedSystemFields');
           fields = _.filter(fields, function(f){
             return !_.include(protectedFields,f);
           });
         }
 
-        this.get('selectedItem').save(fields);
+        this.get('selectedItem').save(fields).catch(function(error) {
+          // this shows the admin-badge-preview modal with the error
+          // kinda weird, but it consolidates the display logic for badge errors
+          self.send('saveError', error);
+        });
       }
     },
 

app/assets/javascripts/admin/routes/admin_badges_route.js

@@ -15,8 +15,39 @@ Discourse.AdminBadgesRoute = Discourse.Route.extend({
   },
 
   actions: {
-    editGroupings: function(model){
+    editGroupings: function(model) {
       Discourse.Route.showModal(this, 'admin_edit_badge_groupings', model);
+    },
+
+    saveError: function(jqXhr) {
+      if (jqXhr.status === 422) {
+        Discourse.Route.showModal(this, 'admin_badge_preview', jqXhr.responseJSON);
+      } else {
+        Em.Logger.error(jqXhr);
+        bootbox.alert(I18n.t('errors.description.unknown'));
+      }
+    },
+
+    preview: function(badge, explain) {
+      var self = this;
+
+      badge.set('preview_loading', true);
+      Discourse.ajax('/admin/badges/preview.json', {
+        method: 'post',
+        data: {
+          sql: badge.query,
+          target_posts: !!badge.target_posts,
+          trigger: badge.trigger,
+          explain: explain
+        }
+      }).then(function(json) {
+        badge.set('preview_loading', false);
+        Discourse.Route.showModal(self, 'admin_badge_preview', json);
+      }).catch(function(error) {
+        badge.set('preview_loading', false);
+        Em.Logger.error(error);
+        bootbox.alert("Network error");
+      });
     }
   }
 

app/assets/javascripts/admin/templates/badges.js.handlebars

@@ -76,7 +76,12 @@
 
       {{#if hasQuery}}
 
-      <a href="/admin/badges/preview" {{action preview this}}>{{i18n admin.badges.preview}}</a>
+      <a href {{action preview this "false"}}>{{i18n admin.badges.preview.link_text}}</a>
+      |
+      <a href {{action preview this "true"}}>{{i18n admin.badges.preview.plan_text}}</a>
+      {{#if preview_loading}}
+        {{i18n loading}}...
+      {{/if}}
 
       <div>
         <span>

app/assets/javascripts/admin/templates/modal/admin_badge_preview.js.handlebars

@@ -0,0 +1,44 @@
+<div class="badge-query-preview">
+  {{#if errors}}
+    <p class="error-header">{{i18n admin.badges.preview.sql_error_header}}</p>
+
+    <div class="badge-errors">
+      {{errors}}
+    </div>
+    <!--
+    TODO we want some help pages for this, link to those instead
+    <p>
+      {{i18n admin.badges.preview.error_help}}
+    </p>
+    <ul>
+      <li><a href="https://meta.discourse.org/t/triggered-custom-badge-queries/19336">https://meta.discourse.org/t/triggered-custom-badge-queries/19336</a></li>
+    </ul>
+    -->
+  {{else}}
+    <p class="grant-count">{{{i18n admin.badges.preview.grant_count count=count}}}</p>
+
+    {{#if count_warning}}
+      <div class="count-warning">
+        <p class="heading"><i class="fa fa-warning"></i> {{i18n admin.badges.preview.bad_count_warning.header}}</p>
+        <p class="body">{{i18n admin.badges.preview.bad_count_warning.text}}</p>
+      </div>
+    {{/if}}
+
+    {{#if sample}}
+      <p class="sample">
+        {{i18n admin.badges.preview.sample}}
+      </p>
+      <ul>
+        {{#each html in processed_sample}}
+          <li>{{{html}}}</li>
+        {{/each}}
+      </ul>
+    {{/if}}
+
+    {{#if has_query_plan}}
+        <div class="badge-query-plan">
+          {{{query_plan_html}}}
+        </div>
+    {{/if}}
+  {{/if}}
+</div>

app/assets/javascripts/admin/views/modals/admin_badge_preview_view.js

@@ -0,0 +1,5 @@
+
+Discourse.AdminBadgePreviewView = Discourse.ModalBodyView.extend({
+  templateName: 'admin/templates/modal/admin_badge_preview',
+  title: I18n.t('admin.badges.preview.modal_title')
+});

app/assets/javascripts/discourse/models/badge.js

@@ -146,10 +146,10 @@ Discourse.Badge = Discourse.Model.extend({
       self.set('savingStatus', I18n.t('saved'));
       self.set('saving', false);
       return self;
-    }, function(error){
+    }).catch(function(error) {
-      self.set('savingStatus', '');
+      self.set('savingStatus', I18n.t('failed'));
       self.set('saving', false);
-      bootbox.alert(error.responseText);
+      throw error;
     });
   },
 

app/assets/stylesheets/common/admin/admin_base.scss

@@ -419,6 +419,38 @@ section.details {
   }
 }
 
+.badge-query-preview {
+  .grant-count, .sample, .error-header {
+    margin-left: 10px;
+  }
+
+  .badge-query-plan, .badge-errors {
+    padding: 4px;
+    background-color: scale-color-diff();
+  }
+
+  .badge-query-plan {
+    font-size: 80%;
+  }
+  .badge-errors {
+    font-family: monospace;
+  }
+
+  .count-warning {
+    background-color: dark-light-diff(rgba($danger,.7), $secondary, 50%, -60%);
+    margin: 0 0 7px 0;
+    padding: 10px 20px;
+
+    p {
+      margin: 0;
+    }
+    .heading {
+      color: $danger;
+      font-weight: bold;
+    }
+  }
+}
+
 // Customise area
 .customize {
   .nav.nav-pills {

app/controllers/admin/badges_controller.rb

@@ -14,7 +14,10 @@ class Admin::BadgesController < Admin::AdminController
   end
 
   def preview
-    render json: BadgeGranter.preview(params[:sql], target_posts: params[:target_posts] == "true")
+    render json: BadgeGranter.preview(params[:sql],
+                                      target_posts: params[:target_posts] == "true",
+                                      explain: params[:explain] == "true",
+                                      trigger: params[:trigger].to_i)
   end
 
   def badge_types
@@ -53,10 +56,29 @@ class Admin::BadgesController < Admin::AdminController
 
   def update
     badge = find_badge
+
+    error = nil
+    Badge.transaction do
       update_badge_from_params(badge)
+
+      # Perform checks to prevent bad queries
+      begin
+        BadgeGranter.contract_checks!(badge.query, { target_posts: badge.target_posts, trigger: badge.trigger })
+      rescue => e
+        # noinspection RubyUnusedLocalVariable
+        error = e.message
+        raise ActiveRecord::Rollback
+      end
+
       badge.save!
+    end
+
+    if error
+      render_json_error error
+    else
       render_serialized(badge, BadgeSerializer, root: "badge")
     end
+  end
 
   def destroy
     find_badge.destroy

app/models/badge.rb

@@ -31,6 +31,18 @@ class Badge < ActiveRecord::Base
     PostRevision = 2
     TrustLevelChange = 4
     UserChange = 8
+
+    def self.is_none?(trigger)
+      [0].include? trigger
+    end
+
+    def self.uses_user_ids?(trigger)
+      [4, 8].include? trigger
+    end
+
+    def self.uses_post_ids?(trigger)
+      [1, 2].include? trigger
+    end
   end
 
   module Queries

app/services/badge_granter.rb

@@ -132,9 +132,38 @@ class BadgeGranter
     "badge_queue".freeze
   end
 
+  # Options:
+  #   :target_posts - whether the badge targets posts
+  #   :trigger - the Badge::Trigger id
+  def self.contract_checks!(sql, opts = {})
+    return unless sql.present?
+    if Badge::Trigger.uses_post_ids?(opts[:trigger])
+      raise "Contract violation:\nQuery triggers on posts, but does not reference the ':post_ids' array" unless sql.match /:post_ids/
+    end
+    if Badge::Trigger.uses_user_ids?(opts[:trigger])
+      raise "Contract violation:\nQuery triggers on users, but does not reference the ':user_ids' array" unless sql.match /:user_ids/
+    end
+    if opts[:trigger] && !Badge::Trigger.is_none?(opts[:trigger])
+      raise "Contract violation:\nQuery is triggered, but does not reference the ':backfill' parameter.\n(Hint: if :backfill is TRUE, you should ignore the :post_ids/:user_ids)" unless sql.match /:backfill/
+    end
+
+    # TODO these three conditions have a lot of false negatives
+    if opts[:target_posts]
+      raise "Contract violation:\nQuery targets posts, but does not return a 'post_id' column" unless sql.match /post_id/
+    end
+    raise "Contract violation:\nQuery does not return a 'user_id' column" unless sql.match /user_id/
+    raise "Contract violation:\nQuery does not return a 'granted_at' column" unless sql.match /granted_at/
+  end
+
+  # Options:
+  #   :target_posts - whether the badge targets posts
+  #   :trigger - the Badge::Trigger id
+  #   :explain - return the EXPLAIN query
   def self.preview(sql, opts = {})
     params = {user_ids: [], post_ids: [], backfill: true}
 
+    BadgeGranter.contract_checks!(sql, opts)
+
     # hack to allow for params, otherwise sanitizer will trigger sprintf
     count_sql = "SELECT COUNT(*) count FROM (#{sql}) q WHERE :backfill = :backfill"
     grant_count = SqlBuilder.map_exec(OpenStruct, count_sql, params).first.count.to_i
@@ -156,23 +185,34 @@ class BadgeGranter
     LIMIT 10"
      end
 
+    query_plan = nil
+    query_plan = ActiveRecord::Base.exec_sql("EXPLAIN #{sql}", params) if opts[:explain]
+
     sample = SqlBuilder.map_exec(OpenStruct, grants_sql, params).map(&:to_h)
 
-    {grant_count: grant_count, sample: sample}
+    sample.each do |result|
+      raise "Query returned a non-existent user ID:\n#{result[:id]}" unless User.find(result[:id]).present?
+      raise "Query did not return a badge grant time\n(Try using 'current_timestamp granted_at')" unless result[:granted_at]
+      if opts[:target_posts]
+        raise "Query did not return a post ID" unless result[:post_id]
+        raise "Query returned a non-existent post ID:\n#{result[:post_id]}" unless Post.find(result[:post_id]).present?
+      end
+    end
+
+    {grant_count: grant_count, sample: sample, query_plan: query_plan}
   rescue => e
-    {error: e.to_s}
+    {errors: e.message}
   end
 
   MAX_ITEMS_FOR_DELTA = 200
   def self.backfill(badge, opts=nil)
     return unless badge.query.present? && badge.enabled
 
+    post_ids = user_ids = nil
+
     post_ids = opts[:post_ids] if opts
     user_ids = opts[:user_ids] if opts
 
-    post_ids = nil unless post_ids.present?
-    user_ids = nil unless user_ids.present?
-
     # safeguard fall back to full backfill if more than 200
     if (post_ids && post_ids.length > MAX_ITEMS_FOR_DELTA) ||
        (user_ids && user_ids.length > MAX_ITEMS_FOR_DELTA)
@@ -180,6 +220,9 @@ class BadgeGranter
       user_ids = nil
     end
 
+    post_ids = nil unless post_ids.present?
+    user_ids = nil unless user_ids.present?
+
     full_backfill = !user_ids && !post_ids
 
     post_clause = badge.target_posts ? "AND (q.post_id = ub.post_id OR NOT :multiple_grant)" : ""

config/locales/client.en.yml

@@ -204,6 +204,7 @@ en:
     disable: "Disable"
     undo: "Undo"
     revert: "Revert"
+    failed: "Failed"
 
     banner:
       close: "Dismiss this banner."
@@ -2024,7 +2025,6 @@ en:
         target_posts: Query targets posts
         auto_revoke: Run revocation query daily
         show_posts: Show post granting badge on badge page
-        preview: Preview badge
         trigger: Trigger
         trigger_type:
           none: "Update daily"
@@ -2032,6 +2032,25 @@ en:
           post_revision: "When a user edits or creates a post"
           trust_level_change: "When a user changes trust level"
           user_change: "When a user is edited or created"
+        preview:
+          link_text: "Preview granted badges"
+          plan_text: "Preview with query plan"
+          modal_title: "Badge Query Preview"
+          sql_error_header: "There was an error with the query."
+          error_help: "See the following links for help with badge queries."
+          bad_count_warning:
+            header: "WARNING!"
+            text: "There are missing grant samples. This happens when the badge query returns user IDs or post IDs that do not exist. This may cause unexpected results later on - please double-check your query."
+          grant_count:
+            zero: "No badges to be assigned."
+            one: "<b>1</b> badge to be assigned."
+            other: "<b>%{count}</b> badges to be assigned."
+          sample: "Sample:"
+          grant:
+            with: <span class="username">%{username}</span>
+            with_post: <span class="username">%{username}</span> for post in %{link}
+            with_post_time: <span class="username">%{username}</span> for post in %{link} at <span class="time">%{time}</span>
+            with_time: <span class="username">%{username}</span> at <span class="time">%{time}</span>
 
     lightbox:
       download: "download"