From 1833b43ae2a7b52813e05a94e5178e1cd7028279 Mon Sep 17 00:00:00 2001 From: riking Date: Mon, 25 Aug 2014 15:17:29 -0700 Subject: [PATCH] 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 --- .../controllers/admin-badge-preview.js.es6 | 49 +++++++++++++++++ .../admin/controllers/admin-badges.js.es6 | 26 +++------ .../admin/routes/admin_badges_route.js | 33 +++++++++++- .../admin/templates/badges.js.handlebars | 7 ++- .../modal/admin_badge_preview.js.handlebars | 44 +++++++++++++++ .../views/modals/admin_badge_preview_view.js | 5 ++ .../javascripts/discourse/models/badge.js | 6 +-- .../stylesheets/common/admin/admin_base.scss | 32 +++++++++++ app/controllers/admin/badges_controller.rb | 30 +++++++++-- app/models/badge.rb | 12 +++++ app/services/badge_granter.rb | 53 +++++++++++++++++-- config/locales/client.en.yml | 21 +++++++- 12 files changed, 285 insertions(+), 33 deletions(-) create mode 100644 app/assets/javascripts/admin/controllers/admin-badge-preview.js.es6 create mode 100644 app/assets/javascripts/admin/templates/modal/admin_badge_preview.js.handlebars create mode 100644 app/assets/javascripts/admin/views/modals/admin_badge_preview_view.js diff --git a/app/assets/javascripts/admin/controllers/admin-badge-preview.js.es6 b/app/assets/javascripts/admin/controllers/admin-badge-preview.js.es6 new file mode 100644 index 00000000000..e141dd24e40 --- /dev/null +++ b/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 = "
";
+
+    _.each(raw, function(linehash) {
+      returned += Handlebars.Utils.escapeExpression(linehash["QUERY PLAN"]);
+      returned += "
";
+    });
+
+    returned += "
"; + 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 = "" + Handlebars.Utils.escapeExpression(grant.title) + ""; + } + + 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); + }) +}); diff --git a/app/assets/javascripts/admin/controllers/admin-badges.js.es6 b/app/assets/javascripts/admin/controllers/admin-badges.js.es6 index 199c7eab1ee..c679db8402b 100644 --- a/app/assets/javascripts/admin/controllers/admin-badges.js.es6 +++ b/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); + }); } }, diff --git a/app/assets/javascripts/admin/routes/admin_badges_route.js b/app/assets/javascripts/admin/routes/admin_badges_route.js index ba8fb4f9c57..4ef15173d36 100644 --- a/app/assets/javascripts/admin/routes/admin_badges_route.js +++ b/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"); + }); } } diff --git a/app/assets/javascripts/admin/templates/badges.js.handlebars b/app/assets/javascripts/admin/templates/badges.js.handlebars index 57d7ae71008..1f0a42725e2 100644 --- a/app/assets/javascripts/admin/templates/badges.js.handlebars +++ b/app/assets/javascripts/admin/templates/badges.js.handlebars @@ -76,7 +76,12 @@ {{#if hasQuery}} - {{i18n admin.badges.preview}} + {{i18n admin.badges.preview.link_text}} + | + {{i18n admin.badges.preview.plan_text}} + {{#if preview_loading}} + {{i18n loading}}... + {{/if}}
diff --git a/app/assets/javascripts/admin/templates/modal/admin_badge_preview.js.handlebars b/app/assets/javascripts/admin/templates/modal/admin_badge_preview.js.handlebars new file mode 100644 index 00000000000..12d78af55d1 --- /dev/null +++ b/app/assets/javascripts/admin/templates/modal/admin_badge_preview.js.handlebars @@ -0,0 +1,44 @@ +
+ {{#if errors}} +

{{i18n admin.badges.preview.sql_error_header}}

+ +
+ {{errors}} +
+ + {{else}} +

{{{i18n admin.badges.preview.grant_count count=count}}}

+ + {{#if count_warning}} +
+

{{i18n admin.badges.preview.bad_count_warning.header}}

+

{{i18n admin.badges.preview.bad_count_warning.text}}

+
+ {{/if}} + + {{#if sample}} +

+ {{i18n admin.badges.preview.sample}} +

+
    + {{#each html in processed_sample}} +
  • {{{html}}}
    • + {{/each}} +
+ {{/if}} + + {{#if has_query_plan}} +
+ {{{query_plan_html}}} +
+ {{/if}} + {{/if}} +
diff --git a/app/assets/javascripts/admin/views/modals/admin_badge_preview_view.js b/app/assets/javascripts/admin/views/modals/admin_badge_preview_view.js new file mode 100644 index 00000000000..0d890b3d663 --- /dev/null +++ b/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') +}); diff --git a/app/assets/javascripts/discourse/models/badge.js b/app/assets/javascripts/discourse/models/badge.js index 3752bf5a57e..43ca8d5e750 100644 --- a/app/assets/javascripts/discourse/models/badge.js +++ b/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){ - self.set('savingStatus', ''); + }).catch(function(error) { + self.set('savingStatus', I18n.t('failed')); self.set('saving', false); - bootbox.alert(error.responseText); + throw error; }); }, diff --git a/app/assets/stylesheets/common/admin/admin_base.scss b/app/assets/stylesheets/common/admin/admin_base.scss index 28342d4b210..748c21ff8a8 100644 --- a/app/assets/stylesheets/common/admin/admin_base.scss +++ b/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 { diff --git a/app/controllers/admin/badges_controller.rb b/app/controllers/admin/badges_controller.rb index cd5170ee98d..00a47fc6705 100644 --- a/app/controllers/admin/badges_controller.rb +++ b/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,9 +56,28 @@ class Admin::BadgesController < Admin::AdminController def update badge = find_badge - update_badge_from_params(badge) - badge.save! - render_serialized(badge, BadgeSerializer, root: "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 diff --git a/app/models/badge.rb b/app/models/badge.rb index 5c398ff5ff3..a31580231df 100644 --- a/app/models/badge.rb +++ b/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 diff --git a/app/services/badge_granter.rb b/app/services/badge_granter.rb index ade8d503fa7..46155e79cea 100644 --- a/app/services/badge_granter.rb +++ b/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)" : "" diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 967de72cf3d..0c7ddc673b0 100644 --- a/config/locales/client.en.yml +++ b/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: "1 badge to be assigned." + other: "%{count} badges to be assigned." + sample: "Sample:" + grant: + with: %{username} + with_post: %{username} for post in %{link} + with_post_time: %{username} for post in %{link} at %{time} + with_time: %{username} at %{time} lightbox: download: "download"