ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

cast columns in dynamic finders, warn on all-nil argument lists 

in production mode we now cast to the appropriate type and issue a warning
if it can't be cleanly cast (e.g. '' -> 0). if all arguments are nil (or
empty arrays, e.g. find_by_id([])) and we aren't in a scope, issue a
warning (sometimes we really do want nil when we're in a scope, e.g. line
216 of app/models/folder/rb).

in development/test mode, we now raise errors in the two warning scenarios
above (though that is configurable).

fixed several places in the code where specs failed due to the change, or
where inputs to dynamic finders looked problematic

Change-Id: Ifea851cb14d3e89b6df08ade8e83934579678f8b
Reviewed-on: https://gerrit.instructure.com/3434
Reviewed-by: Zach Wily <zach@instructure.com>
Reviewed-by: Brian Palmer <brianp@instructure.com>
Tested-by: Hudson <hudson@instructure.com>
This commit is contained in:
Jon Jensen 2011-05-02 23:56:39 -06:00 committed by Zach Wily
parent 133cc232fe
commit 6d226272df
23 changed files with 107 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
app/controllers/application_controller.rb
View File

@ -269,8 +269,10 @@ class ApplicationController < ActionController::Base
# find only those courses and groups passed in the only_contexts
# parameter, but still scoped by user so we know they have rights to
# view them.
courses = courses.find_all_by_id(only_contexts.select { |c| c.first == "Course" }.map(&:last))
groups = groups.find_all_by_id(only_contexts.select { |c| c.first == "Group" }.map(&:last)) if include_groups
course_ids = only_contexts.select { |c| c.first == "Course" }.map(&:last)
courses = course_ids.empty? ? [] : courses.find_all_by_id(course_ids)
group_ids = only_contexts.select { |c| c.first == "Group" }.map(&:last)
groups = group_ids.empty? ? [] : groups.find_all_by_id(group_ids) if include_groups
end
@contexts.concat courses
@contexts.concat groups
@ -410,7 +412,7 @@ class ApplicationController < ActionController::Base
@context = nil
@problem = nil
if pieces[0] == "enrollment"
@enrollment = Enrollment.find_by_uuid(pieces[1])
@enrollment = Enrollment.find_by_uuid(pieces[1]) if pieces[1]
@context_type = "Course"
if !@enrollment
@problem = "The verification code does not match any currently enrolled user."
@ -420,7 +422,7 @@ class ApplicationController < ActionController::Base
@context = @enrollment.course unless @problem
@current_user = @enrollment.user unless @problem
elsif pieces[0] == 'group_membership'
@membership = GroupMembership.find_by_uuid(pieces[1])
@membership = GroupMembership.find_by_uuid(pieces[1]) if pieces[1]
@context_type = "Group"
if !@membership
@problem = "The verification code does not match any currently enrolled user."
@ -433,7 +435,7 @@ class ApplicationController < ActionController::Base
@context_type = pieces[0].classify
if Context::ContextTypes.const_defined?(@context_type)
@context_class = Context::ContextTypes.const_get(@context_type)
@context = @context_class.find_by_uuid(pieces[1])
@context = @context_class.find_by_uuid(pieces[1]) if pieces[1]
end
if !@context
@problem = "The verification code is invalid."

6
app/controllers/assignment_groups_controller.rb
View File

@ -107,7 +107,7 @@ class AssignmentGroupsController < ApplicationController
@group = @context.assignment_groups.find(params[:assignment_group_id])
if authorized_action(@group, @current_user, :update)
order = params[:order].split(',').map{|id| id.to_i }
group_ids = ([@group.id] + @context.assignments.find_all_by_id(order).map(&:assignment_group_id)).uniq.compact
group_ids = ([@group.id] + order.empty? ? [] : @context.assignments.find_all_by_id(order).map(&:assignment_group_id)).uniq.compact
Assignment.update_all("assignment_group_id=#{@group.id}", :id => order, :context_id => @context.id, :context_type => @context.class.to_s)
@group.assignments.first.update_order(order) unless @group.assignments.empty?
AssignmentGroup.update_all({:updated_at => Time.now}, {:id => group_ids})
@ -179,8 +179,8 @@ class AssignmentGroupsController < ApplicationController
order = @new_group.assignments.active.map(&:id)
ids_to_change = @assignment_group.assignments.active.map(&:id)
order += ids_to_change
Assignment.update_all({:assignment_group_id => @new_group.id, :updated_at => Time.now}, {:id => ids_to_change})
Assignment.find_by_id(order).update_order(order)
Assignment.update_all({:assignment_group_id => @new_group.id, :updated_at => Time.now}, {:id => ids_to_change}) unless ids_to_change.empty?
Assignment.find_by_id(order).update_order(order) unless order.empty?
@new_group.touch
@assignment_group.reload
end

5
app/controllers/content_imports_controller.rb
View File

@ -105,8 +105,9 @@ class ContentImportsController < ApplicationController
def migrate_content_execute
if authorized_action(@context, @current_user, :manage)
@content_migration = ContentMigration.find_by_context_id_and_context_type_and_id(@context.id, @context.class.to_s, params[:id] || params[:copy][:content_migration_id]) rescue nil
@content_migration ||= ContentMigration.find_all_by_context_id_and_context_type(@context.id, @context.class.to_s).last
migration_id = params[:id] || params[:copy] && params[:copy][:content_migration_id]
@content_migration = ContentMigration.find_by_context_id_and_context_type_and_id(@context.id, @context.class.to_s, migration_id) if migration_id.present?
@content_migration ||= ContentMigration.find_by_context_id_and_context_type(@context.id, @context.class.to_s, :order => "id DESC")
if request.method == :post
@content_migration.migration_settings[:migration_ids_to_import] = params
@content_migration.save

2
app/controllers/gradebooks_controller.rb
View File

@ -100,7 +100,7 @@ class GradebooksController < ApplicationController
protected :submissions_json
def attendance
@enrollment = @context.all_student_enrollments.find_by_user_id(params[:user_id])
@enrollment = @context.all_student_enrollments.find_by_user_id(params[:user_id]) if params[:user_id].present?
@enrollment ||= @context.all_student_enrollments.find_by_user_id(@current_user.id) if !@context.grants_right?(@current_user, session, :manage_grades)
add_crumb 'Attendance'
if !@enrollment && @context.grants_right?(@current_user, session, :manage_grades)

9
app/controllers/quizzes_controller.rb
View File

@ -59,7 +59,8 @@ class QuizzesController < ApplicationController
add_crumb(@quiz.title, named_context_url(@context, :context_quiz_url, @quiz))
add_crumb("Statistics", named_context_url(@context, :context_quiz_statistics_url, @quiz))
@statistics = @quiz.statistics(params[:all_versions] == '1')
@submitted_users = User.find_all_by_id(@quiz.quiz_submissions.select{|s| !s.settings_only? }.map(&:user_id)).compact.uniq.sort_by(&:last_name_first)
user_ids = @quiz.quiz_submissions.select{|s| !s.settings_only? }.map(&:user_id)
@submitted_users = user_ids.empty? ? [] : User.find_all_by_id(user_ids).compact.uniq.sort_by(&:last_name_first)
}
format.csv {
send_data(
@ -80,8 +81,10 @@ class QuizzesController < ApplicationController
student_ids = @context.students.map{|s| s.id }
@banks_hash = {}
bank_ids = @quiz.quiz_groups.map(&:assessment_question_bank_id)
AssessmentQuestionBank.active.find_all_by_id(bank_ids).compact.each do |bank|
@banks_hash[bank.id] = bank
unless bank_ids.empty?
AssessmentQuestionBank.active.find_all_by_id(bank_ids).compact.each do |bank|
@banks_hash[bank.id] = bank
end
end
if @has_student_submissions = @quiz.has_student_submissions?
flash[:notice] = "Keep in mind, some students have already taken or started taking this quiz"

2
app/controllers/sections_controller.rb
View File

@ -41,7 +41,7 @@ class SectionsController < ApplicationController
course_id = params[:new_course_id]
# cross-listing should only be allowed within the same root account
@new_course = @section.root_account.all_courses.find_by_id(course_id) if course_id.present?
@new_course ||= @section.root_account.all_courses.find_by_sis_source_id(course_id)
@new_course ||= @section.root_account.all_courses.find_by_sis_source_id(course_id) if course_id.present?
allowed = @new_course && @section.grants_right?(@current_user, session, :update) && @new_course.grants_right?(@current_user, session, :manage_admin_users)
res = {:allowed => !!allowed}
if allowed

3
app/controllers/submissions_controller.rb
View File

@ -74,7 +74,8 @@ class SubmissionsController < ApplicationController
format.html { redirect_to @attachment.cacheable_s3_url }
elsif params[:download]
@attachment = @submission.attachment if @submission.attachment_id == params[:download].to_i
@attachment ||= Attachment.find_by_id(@submission.submission_history.map(&:attachment_id).find{|a| a == params[:download].to_i })
prior_attachment_id = @submission.submission_history.map(&:attachment_id).find{|a| a == params[:download].to_i }
@attachment ||= Attachment.find_by_id(prior_attachment_id) if prior_attachment_id
@attachment ||= @submission.attachments.find_by_id(params[:download]) if params[:download].present?
@attachment ||= @submission.submission_history.map(&:versioned_attachments).flatten.find{|a| a.id == params[:download].to_i }
@attachment ||= @submission.attachment if @submission.attachment_id == params[:download].to_i

4
app/controllers/users_controller.rb
View File

@ -480,7 +480,7 @@ class UsersController < ApplicationController
end
def merge
@user_about_to_go_away = User.find_by_uuid(session[:merge_user_uuid])
@user_about_to_go_away = User.find_by_uuid(session[:merge_user_uuid]) if session[:merge_user_uuid].present?
@user_about_to_go_away = nil unless @user_about_to_go_away.id == params[:user_id].to_i
if params[:new_user_uuid] && @true_user = User.find_by_uuid(params[:new_user_uuid])
@ -533,7 +533,7 @@ class UsersController < ApplicationController
end
def confirm_merge
@user = User.find_by_uuid(session[:merge_user_uuid])
@user = User.find_by_uuid(session[:merge_user_uuid]) if session[:merge_user_uuid].present?
@user = nil unless @user && @user.id == session[:merge_user_id]
if @user && @user != @current_user
render :action => 'confirm_merge'

2
app/models/assessment_question_bank.rb
View File

@ -81,7 +81,7 @@ class AssessmentQuestionBank < ActiveRecord::Base
def select_for_submission(count)
ids = ActiveRecord::Base.connection.select_all("SELECT id FROM assessment_questions WHERE workflow_state != 'deleted' AND assessment_question_bank_id = #{self.id}")
ids = ids.sort_by{rand}[0...count].map{|i|i['id']}
AssessmentQuestion.find_all_by_id(ids)
ids.empty? ? [] : AssessmentQuestion.find_all_by_id(ids)
end
def outcomes=(hash)

4
app/models/assignment.rb
View File

@ -836,7 +836,7 @@ class Assignment < ActiveRecord::Base
raise "No submission found for that student" unless submission
group, students = group_students(original_student)
opts[:unique_key] = Time.now.to_s
opts[:author] ||= opts[:commenter] || User.find_by_id(opts[:user_id])
opts[:author] ||= opts[:commenter] || opts[:user_id].present? && User.find_by_id(opts[:user_id])
opts[:anonymous] = opts[:author] != original_student && self.anonymous_peer_reviews && !self.grants_right?(opts[:author], nil, :grade)
if opts[:comment] && opts[:assessment_request]
@ -1502,7 +1502,7 @@ class Assignment < ActiveRecord::Base
attachment_id = nil if filename.match(/\A\._/)
user_id = split_filename[-2].to_i
user = User.find_by_id(user_id)
attachment = Attachment.find_by_id(attachment_id) rescue nil
attachment = attachment_id && Attachment.find_by_id(attachment_id)
submission = Submission.find_by_user_id_and_assignment_id(user_id, self.id)
if !attachment || !submission
@ignored_files << filename

4
app/models/attachment.rb
View File

@ -295,10 +295,10 @@ class Attachment < ActiveRecord::Base
if !self.scribd_mime_type_id
@@mime_ids ||= {}
@@mime_ids[self.after_extension] ||= ScribdMimeType.find_by_extension(self.after_extension).try(:id)
@@mime_ids[self.after_extension] ||= self.after_extension && ScribdMimeType.find_by_extension(self.after_extension).try(:id)
self.scribd_mime_type_id = @@mime_ids[self.after_extension]
if !self.scribd_mime_type_id
@@mime_ids[self.content_type] ||= ScribdMimeType.find_by_name(self.content_type).try(:id)
@@mime_ids[self.content_type] ||= self.content_type && ScribdMimeType.find_by_name(self.content_type).try(:id)
self.scribd_mime_type_id = @@mime_ids[self.content_type]
end
end

1
app/models/context_message.rb
View File

@ -128,6 +128,7 @@ class ContextMessage < ActiveRecord::Base
sender = self.context_message_participants.find_by_user_id_and_participation_type(self.user_id, 'sender')
sender ||= self.context_message_participants.create(:user_id => self.user_id, :participation_type => 'sender')
recipient_users = self.context.users.select{|u| (self.recipients || []).include?(u.id) }
return if recipient_users.empty?
participants_hash = {}
self.context_message_participants.find_all_by_user_id(recipient_users.map(&:id)).each do |participant|
participants_hash[participant.user_id] = true

2
app/models/context_module.rb
View File

@ -81,7 +81,7 @@ class ContextModule < ActiveRecord::Base
original_position ||= self.position || 0
positions = ContextModule.module_positions(self.context).to_a.sort_by{|a| a[1] }
downstream_ids = positions.select{|a| a[1] > (self.position || 0)}.map{|a| a[0] }
downstreams = self.context.context_modules.active.find_all_by_id(downstream_ids)
downstreams = downstream_ids.empty? ? [] : self.context.context_modules.active.find_all_by_id(downstream_ids)
downstreams.each {|m| m.save_without_touching_context }
end

4
app/models/course.rb
View File

@ -502,7 +502,7 @@ class Course < ActiveRecord::Base
belongs_to :account
def wiki
res = Wiki.find_by_id(self.wiki_id)
res = self.wiki_id && Wiki.find_by_id(self.wiki_id)
unless res
res = WikiNamespace.default_for_context(self).wiki
self.wiki_id = res.id if res
@ -856,7 +856,7 @@ class Course < ActiveRecord::Base
enrollments = self.student_enrollments.scoped({:include => [:user, :course_section]}).find(:all, :order => "users.sortable_name")
enrollment_ids = []
publishing_pseudonym = publishing_user.pseudonyms.active.find_all_by_account_id(self.root_account_id, :order => "sis_user_id DESC").first
publishing_pseudonym = publishing_user.pseudonyms.active.find_by_account_id(self.root_account_id, :order => "sis_user_id DESC")
res = FasterCSV.generate do |csv|

4
app/models/discussion_topic.rb
View File

@ -563,12 +563,12 @@ class DiscussionTopic < ActiveRecord::Base
end
media_object_ids = media_object_ids.uniq.compact
attachment_ids = attachment_ids.uniq.compact
attachments = context.attachments.active.find_all_by_id(attachment_ids).compact
attachments = attachment_ids.empty? ? [] : context.attachments.active.find_all_by_id(attachment_ids).compact
attachments = attachments.select{|a| a.content_type && a.content_type.match(/(video|audio)/) }
attachments.each do |attachment|
attachment.podcast_associated_asset = messages_hash[attachment.id]
end
media_objects = MediaObject.find_all_by_media_id(media_object_ids)
media_objects = media_object_ids.empty? ? [] : MediaObject.find_all_by_media_id(media_object_ids)
media_objects += media_object_ids.map{|id| MediaObject.new(:media_id => id) }
media_objects = media_objects.once_per(&:media_id)
media_objects = media_objects.map do |media_object|

2
app/models/group.rb
View File

@ -66,7 +66,7 @@ class Group < ActiveRecord::Base
adheres_to_policy
def wiki
res = Wiki.find_by_id(self.wiki_id)
res = self.wiki_id && Wiki.find_by_id(self.wiki_id)
unless res
res = WikiNamespace.default_for_context(self).wiki
self.wiki_id = res.id if res

12
app/models/learning_outcome_group.rb
View File

@ -51,8 +51,10 @@ class LearningOutcomeGroup < ActiveRecord::Base
tags.each{|t| positions[t.content_asset_string] = t.position }
ids_to_find = tags.select{|t| t.content_type == 'LearningOutcome'}.map(&:content_id)
ids_to_find = (ids_to_find & outcome_ids) unless outcome_ids.empty?
objects = LearningOutcome.active.find_all_by_id(ids_to_find).compact
objects += LearningOutcomeGroup.active.find_all_by_id(tags.select{|t| t.content_type == 'LearningOutcomeGroup'}.map(&:content_id)).compact
group_ids_to_find = tags.select{|t| t.content_type == 'LearningOutcomeGroup'}.map(&:content_id)
objects = []
objects += LearningOutcome.active.find_all_by_id(ids_to_find).compact unless ids_to_find.empty?
objects += LearningOutcomeGroup.active.find_all_by_id(group_ids_to_find).compact unless group_ids_to_find.empty?
if self.learning_outcome_group_id == nil
all_tags = all_tags_for_context
codes = all_tags.map(&:content_asset_string).uniq
@ -137,8 +139,10 @@ class LearningOutcomeGroup < ActiveRecord::Base
tag.associated_asset = self
tag.save!
group = tag.content
outcomes = LearningOutcome.find_all_by_id(item.content_tags.select{|t| t.content_type == 'LearningOutcome'}.map(&:content_id))
outcomes.each{|o| group.add_item(o) if !opts[:only] || opts[:only][o.id] == "1" }
outcome_ids = item.content_tags.select{|t| t.content_type == 'LearningOutcome'}.map(&:content_id)
unless outcome_ids.empty?
LearningOutcome.find_all_by_id(outcome_ids).each{|o| group.add_item(o) if !opts[:only] || opts[:only][o.id] == "1" }
end
tag
end
end

2
app/models/quiz_submission.rb
View File

@ -90,7 +90,7 @@ class QuizSubmission < ActiveRecord::Base
def track_outcomes(attempt)
return unless user_id
question_ids = (self.quiz_data || []).map{|q| q[:assessment_question_id] }.compact.uniq
questions = AssessmentQuestion.find_all_by_id(question_ids).compact
questions = question_ids.empty? ? [] : AssessmentQuestion.find_all_by_id(question_ids).compact
bank_ids = questions.map(&:assessment_question_bank_id).uniq
tagged_bank_ids = (bank_ids.empty? ? [] : ContentTag.outcome_tags_for_banks(bank_ids).scoped(:select => 'content_id')).map(&:content_id).uniq
if !tagged_bank_ids.empty?

9
app/models/submission.rb
View File

@ -297,8 +297,9 @@ class Submission < ActiveRecord::Base
ids.uniq!
existing_associations = associations.select{|a| ids.include?(a.attachment_id) }
(associations - existing_associations).each{|a| a.destroy }
associated_ids = ids.select{|id| association_ids.include?(id) }
attachments = Attachment.find_all_by_id(ids - associated_ids)
unassociated_ids = ids.reject{|id| association_ids.include?(id) }
return if unassociated_ids.empty?
attachments = Attachment.find_all_by_id(unassociated_ids)
attachments.each do |a|
if((a.context_type == 'User' && a.context_id == user_id) ||
(a.context_type == 'Group' && a.context_id == group_id) ||
@ -306,9 +307,6 @@ class Submission < ActiveRecord::Base
self.attachment_associations.find_or_create_by_attachment_id(a.id)
end
end
(ids - associated_ids).each{|id|
self.attachment_associations.find_or_create_by_attachment_id(id)
}
end
def set_context_code
@ -406,6 +404,7 @@ class Submission < ActiveRecord::Base
def versioned_attachments
ids = (self.attachment_ids || "").split(",").map{|id| id.to_i}
ids << self.attachment_id if self.attachment_id
return [] if ids.empty?
Attachment.find_all_by_id(ids).select{|a|
(a.context_type == 'User' && a.context_id == user_id) ||
(a.context_type == 'Group' && a.context_id == group_id) ||

5
config/environments/development.rb
View File

@ -20,6 +20,11 @@ config.action_mailer.raise_delivery_errors = true
# they hit.
Canvas.protected_attribute_error = :raise
# Raise an exception on finder type mismatch or nil arguments. Helps us catch
# these bugs before they hit.
Canvas.dynamic_finder_nil_arguments_error = :raise
Canvas.dynamic_finder_type_cast_error = :raise
SslRequirement.ssl_host = "localhost:3000"
SslRequirement.standard_host = "localhost:3000"
SslRequirement.disable_ssl_check = true

5
config/environments/test.rb
View File

@ -25,6 +25,11 @@ config.action_mailer.delivery_method = :test
# they hit.
Canvas.protected_attribute_error = :raise
# Raise an exception on finder type mismatch or nil arguments. Helps us catch
# these bugs before they hit.
Canvas.dynamic_finder_nil_arguments_error = :raise
Canvas.dynamic_finder_type_cast_error = :raise
# Inject our Rails 2.3.x broken cookie fix. See the whole sordid tale
# here:
# https://rails.lighthouseapp.com/projects/8994/tickets/4743-session-cookie-breaks-if-used-with-custom-cookie-in-rails-238

30
config/initializers/active_record.rb
View File

@ -253,6 +253,36 @@ class ActiveRecord::Base
end
sanitize_sql_array ["(" + cols.join(" OR ") + ")", *([value] * cols.size)]
end
class DynamicFinderTypeError < Exception; end
class << self
def construct_attributes_from_arguments_with_type_cast(attribute_names, arguments)
log_dynamic_finder_nil_arguments(attribute_names) if current_scoped_methods.nil? && arguments.flatten.compact.empty?
attributes = construct_attributes_from_arguments_without_type_cast(attribute_names, arguments)
attributes.each_pair do |attribute, value|
next unless column = columns.detect{ |col| col.name == attribute.to_s }
next if [value].flatten.compact.empty?
cast_value = [value].flatten.map{ |v| v.respond_to?(:quoted_id) ? v : column.type_cast(v) }
cast_value = cast_value.first unless value.is_a?(Array)
next if [value].flatten.map(&:to_s) == [cast_value].flatten.map(&:to_s)
log_dynamic_finder_type_cast(value, column)
attributes[attribute] = cast_value
end
end
alias_method_chain :construct_attributes_from_arguments, :type_cast
def log_dynamic_finder_nil_arguments(attribute_names)
error = "No non-nil arguments passed to #{self.base_class}.find_by_#{attribute_names.join('_and_')}"
raise DynamicFinderTypeError, error if Canvas.dynamic_finder_nil_arguments_error == :raise
logger.debug "WARNING: " + error
end
def log_dynamic_finder_type_cast(value, column)
error = "Cannot cleanly cast #{value.inspect} to #{column.type} (#{self.base_class}\##{column.name})"
raise DynamicFinderTypeError, error if Canvas.dynamic_finder_type_cast_error == :raise
logger.debug "WARNING: " + error
end
end
end
class ActiveRecord::Serialization::Serializer

17
lib/canvas.rb
View File

@ -4,6 +4,23 @@ module Canvas
# this to :raise to raise an exception.
mattr_accessor :protected_attribute_error
# defines the behavior around casting arguments passed into dynamic finders.
# Arguments are coerced to the appropriate type (if the column exists), so
# things like find_by_id('123') become find_by_id(123). The default (:log)
# logs a warning if the cast isn't clean (e.g. '123a' -> 123 or '' -> 0).
# Set this to :raise to raise an error on unclean casts.
mattr_accessor :dynamic_finder_type_cast_error
# defines the behavior when nil or empty array arguments passed into dynamic
# finders. The default (:log) logs a warning if the finder is not scoped and
# if *all* arguments are nil/[], e.g.
# Thing.find_by_foo_and_bar(nil, nil) # warning
# Other.find_by_baz([]) # warning
# Another.find_all_by_a_and_b(123, nil) # ok
# ThisThing.some_scope.find_by_whatsit(nil) # ok
# Set this to :raise to raise an exception.
mattr_accessor :dynamic_finder_nil_arguments_error
def self.active_record_foreign_key_check(name, type, options)
if name.to_s =~ /_id\z/ && type.to_s == 'integer' && options[:limit].to_i < 8
raise ArgumentError, "All foreign keys need to be 8-byte integers. #{name} looks like a foreign key to me, please add this option: `:limit => 8`"