From 9cc6fbeaebb589f913048485f775387b559fbbe8 Mon Sep 17 00:00:00 2001 From: Yacine Petitprez Date: Thu, 9 Aug 2018 16:51:42 +0800 Subject: [PATCH] - Better error display for 80% of the errors occured in Clear. --- CHANGELOG.md | 2 + spec/model/model_spec.cr | 3 + spec/sql/delete_spec.cr | 2 - src/clear.cr | 1 + src/clear/error_messages.cr | 282 ++++++++++++++++++ src/clear/extensions/bcrypt/bcrypt.cr | 2 +- src/clear/extensions/enum/enum.cr | 4 +- .../full_text_searchable/tsvector.cr | 2 +- src/clear/migration/manager.cr | 21 +- src/clear/migration/migration.cr | 9 +- src/clear/model/column.cr | 10 +- src/clear/model/model.cr | 3 +- src/clear/model/modules/is_polymorphic.cr | 4 +- src/clear/sql/delete_query.cr | 2 +- src/clear/sql/fragment/from.cr | 4 +- src/clear/sql/fragment/join.cr | 2 +- src/clear/sql/query/having.cr | 4 +- src/clear/sql/query/order_by.cr | 2 +- src/clear/sql/query/where.cr | 4 +- src/clear/sql/sql.cr | 3 +- src/clear/sql/update_query.cr | 2 +- src/clear/version.cr | 2 +- 22 files changed, 328 insertions(+), 42 deletions(-) create mode 100644 src/clear/error_messages.cr diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c52b34f2..12604664d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,10 +11,12 @@ For example, if A belongs to B, B belongs to C, then A has_many C through B. You can perform this now without declaring any class for B; see the guide about relations for more informations. +- Add error messages so cool you want your code to crash đŸ˜‰ ## Bug fixes - Fix #23 bug with `has_many through:` and select - Add support for `DISTINCT ON` feature. +- Array(String), Array(Int64) columns type are working now works ## Breaking changes - `Model#save` on read only model do not throw exception anymore but return false (save! still throw error) diff --git a/spec/model/model_spec.cr b/spec/model/model_spec.cr index 72cb9edbe..cdc918e9b 100644 --- a/spec/model/model_spec.cr +++ b/spec/model/model_spec.cr @@ -446,6 +446,9 @@ module ModelSpec end context "with pagination" do + it "test array" do + end + it "can pull the next 5 users from page 2" do temporary do reinit diff --git a/spec/sql/delete_spec.cr b/spec/sql/delete_spec.cr index 64633fef9..f8d9e1e5f 100644 --- a/spec/sql/delete_spec.cr +++ b/spec/sql/delete_spec.cr @@ -1,5 +1,3 @@ -require "spec" - require "../spec_helper" module DeleteSpec diff --git a/src/clear.cr b/src/clear.cr index 8db943e00..149fb3a5d 100644 --- a/src/clear.cr +++ b/src/clear.cr @@ -22,6 +22,7 @@ end require "./clear/version" require "./clear/util" +require "./clear/error_messages" require "./clear/seed" require "./clear/expression/**" require "./clear/sql/**" diff --git a/src/clear/error_messages.cr b/src/clear/error_messages.cr new file mode 100644 index 000000000..9504818ac --- /dev/null +++ b/src/clear/error_messages.cr @@ -0,0 +1,282 @@ +# Declaration of the differents error messages. +# I try to make Clear as dev-friendly as possible +# I put it here so it doesn't overflow through the code + +module Clear::ErrorMessages + extend self + + private def build_url(url) + url.colorize.underline.to_s + end + + def format_width(x, w = 80) + counter = 0 + o = [] of String + new_line = false + x.split(/([ \n\t])/).each do |word| + case word + when "\n" + o << word + counter = 0 + else + counter += word.size + + if counter > w + o << "\n" + if word == " " + counter = 0 + else + o << word + counter = word.size + end + else + o << word + end + end + + end + + o.join + end + + private def build_tips(ways_to_resolve) + if ways_to_resolve.size > 0 + "Here some tips:\n\n" + + ways_to_resolve.map do |l| + l = format_width(l, 72) + l = " - " + l.gsub(/\n/){ |m| "#{m} "} + end.join("\n\n") + "\n\n" + end + end + + private def build_message(message) + "#{message}\n\n" + end + + private def build_manual(manual_pages) + if manual_pages.size > 0 + { + "You may want to check the manual:", + manual_pages.map{ |x| + build_url("https://github.com/anykeyh/clear/tree/master/manual/#{x}") + }.join("\n") + }.join("\n") + "\n\n" + end + end + + def build_error_message(message : String, ways_to_resolve : Tuple = Tuple.new, manual_pages : Tuple = Tuple.new) + format_width({ + build_message(message), + build_tips(ways_to_resolve), + build_manual(manual_pages), + ( + "Your may also have encountered a bug. \n"+ + "Feel free to fill an issue: \n#{build_url("https://github.com/anykeyh/clear/issues/new")}" + ), + "\n\nStack trace:\n" + }.join) + end + + def migration_already_up(number) + build_error_message \ + "Migration already up: #{number}", + { + "You try to force a migration which is already existing in your database. You should down the migration first, then up it again." + }, + { + "migration/Migration.md" + } + end + + def migration_already_down(number) + build_error_message \ + "Migration already down: #{number}", + { + "You try to force a migration which is not set in your database yet. You should up the migration first, then down it again." + }, + { + "migration/Migration.md" + } + end + + def migration_not_found(number) + build_error_message \ + "The migration number `#{number}` is not found.", + { + "Ensure to require your migration files", + "Number of the migrations can be found in the filename, in the classname or in the `uid` method of the migration." + }, + { + "migration/Migration.md" + } + end + + def no_migration_yet(version) + build_error_message \ + "No migration are registered yet, so we cannot go to version=#{version}", + { + "Ensure to require your migration files", + "Ensure you have some migration files. Captain obvious to the rescue! ;-)" + }, + { + "migration/Migration.md" + } + end + + def uid_not_found(class_name) + build_error_message \ + "I don't know how to order the migration `#{class_name}`", + { + "Rename your migration class to have the migration UID at the end of the class name", + "Rename the file where your migration stand to have the migration UID in front of the filename", + "Override the method `uid`. Be sure the number is immutable (e.g. return constant)" + }, + { + "migration/Migration.md" + } + end + + def migration_irreversible(name) + build_error_message \ + "The migration #{name} is irreversible. You're trying to down a migration which is not downable, because the operations are one way only.", + { + "Build a way to revert the migration", + "Do not revert the migration", + "Maybe you need to manually flush the migration using Postgres. `__clear_metadatas` table store loaded migrations. Good luck !" + }, + { + "migration/Migration.md" + } + end + + def migration_drop_irreversible(name) + build_error_message \ + "Cannot revert column drop, because datatype is unknown", + { + "Add the optional previous data `type` argument in the operation `drop`:" + + "`drop_column(table, column, type)`" + }, + { + "migration/Migration.md" + } + end + + def migration_not_unique(numbers) + build_error_message \ + "The migration manage found collision on migration number. Migrations number are: #{numbers.join(", ")}", + { + "It happens when migration share the same `uid`. Try to change the UID of one of your migrations", + "By default, Clear has a `-1` migration used internally. Do not use this migration number.", + "Migration numbers can be found in filename, classname or return of `uid` method" + }, + { + "migration/Migration.md" + } + end + + def illegal_setter_access_to_undefined_column(name) + build_error_message \ + "You're trying to access to the column `#{name}` but it is not initialized.", + { + "Ensure than the column `#{name}` exists in your table", + "If the model comes from a collection query, there was maybe a filtering on your `select` clause, and you forgot to declare the column `#{name}`", + "In the case of unpersisted models, please initialize by calling `#{name}=` first", + "For validator, try `ensure_than` method, or use `#{name}_column.defined?` to avoid your validation code.", + "Are you calling `#{name}_column.revert` somewhere before?", + "If your model comes from JSON, please ensure the JSON source defines the column. Usage of `strict` mode will trigger exception on JSON loading." + }, + { + "model/Definition.md", + "model/Lifecycle.md" + } + end + + def null_column_mapping_error(name, type) + build_error_message \ + "Your field `#{name}` is declared as `#{type}` but `NULL` value has been found in the database.", + { + "In your model, declare your column `column #{name} : #{type}?` (note the `?` which allow nil value)", + "In your database, adding `DEFAULT` value and/or `NOT NULL` constraint should disallow NULL fields from your data." + }, + { + "model/Definition.md#presence-validation" + } + end + + def converter_error(from, to) + build_error_message \ + "Clear cannot convert from `#{from}` to #{to}.", + { + "Ensure your database column type match the column declaration in Clear" + }, + { "model/Definition.md" } + end + + def lack_of_primary_key(model_name) + build_error_message \ + "Model `#{model_name}` lacks of primary key field", + { + "Define a column as primary key", + "Only one column can be primary key (no compound keys are allowed in Clear for now)", + "You can use the helpers for primary key (see manual page)" + }, + { "model/PrimaryKeyTweaking.md" } + end + + def polymorphic_nil(through) + build_error_message \ + "Impossible to instantiate polymorphic object, because the type given by the data is nil.", + { + "The column `#{through}` contains NULL value, but is set as storage for "+ + "the type of the polymorphic object.", + "Try to set DEFAULT value for your column `#{through}`", + "In case of new implementation of polymorphic system, we recommend you to update the column to the previous Class value. Value must be equal to the fully qualified model class name in Crystal Lang (e.g. `MyApp::MyModel`)" + }, + { "model/Polymorphism.md" } + end + + def polymorphic_unknown_class(class_name) + build_error_message \ + "Impossible to instantiate a new `#{class_name}` using polymorphism.", + { + "Ensure the type is properly setup in your `polymorphic` helper. "+ + "Any model which can exists in your database needs to manually be setup as in the example below:\n"+ + "`polymorphic Dog, Cat, through: \"type\"`\n"+ + "In this case, if you have a `Cow` object in your database, then add it in the list of allowed polymorphic objects.", + "Ensure the name match a fully qualified, with full path, Clear model:\n" + + "`polymorphic ::Animal::Dog, ::Animal::Cat, through: \"type\"`\n" + + "The column should then contains `Animal::Dog` and not `Dog`" + }, + { + "model/Polymorphism.md" + } + end + + def order_by_error_invalid_order(current_order) + build_error_message \ + "Order by allow only ASC and DESC directions. But #{current_order} was given.", + { + "Ensure to use :asc, :desc symbol (or string) when constructing your query.", + "If the code is dynamic, force the casting to one of the two value above, to avoid SQL injection." + }, + { + "querying/RequestBuilding.md" + } + end + + def query_building_error(message) + Clear::SQL::QueryBuildingError.new( + build_error_message({"You're trying to construct an invalid SQL request:\n", + message}.join, manual_pages: {"querying/RequestBuilding.md"} ) + ) + end + + def uninitialized_db_connection(connection) + build_error_message("Your trying to access to the connection #{connection} which is not initialized", + { + "Use `Clear::SQL.init(#{connection}: \"postgres://XXX...\" )` on startup of your application", + "The name of the connection (#{connection}) can't be found. It may have been mistyped." + }, {"Setup.md","model/MultiConnection.md"}) + end + +end \ No newline at end of file diff --git a/src/clear/extensions/bcrypt/bcrypt.cr b/src/clear/extensions/bcrypt/bcrypt.cr index 7269deb97..647a6d9e9 100644 --- a/src/clear/extensions/bcrypt/bcrypt.cr +++ b/src/clear/extensions/bcrypt/bcrypt.cr @@ -23,7 +23,7 @@ module Clear::Model::Converter::Crypto::Bcrypt::PasswordConverter when Nil return nil else - raise "Cannot convert #{x.class} to Crypto::Bcrypt::Password" + raise Clear::ErrorMessages.converter_error(x.class.name, "Crypto::Bcrypt::Password") end end diff --git a/src/clear/extensions/enum/enum.cr b/src/clear/extensions/enum/enum.cr index e540d6573..78e7a89a1 100644 --- a/src/clear/extensions/enum/enum.cr +++ b/src/clear/extensions/enum/enum.cr @@ -35,7 +35,7 @@ module Clear when Nil return nil else - raise "Cannot convert #{x.class} to Enum : #{T.class}" + raise converter_error(x.class.name, "Enum: #{T.class.name}") end end end @@ -92,7 +92,7 @@ module Clear when Slice(UInt8) ::\{{@type}}.from_string(String.new(x)) else - raise "Cannot convert #{x.class} to ::{{@type}}" + raise Clear::ErrorMessages.converter_error(x.class.name, "::{{@type}}") end end diff --git a/src/clear/extensions/full_text_searchable/tsvector.cr b/src/clear/extensions/full_text_searchable/tsvector.cr index 6a15b93ff..6e3cb078a 100644 --- a/src/clear/extensions/full_text_searchable/tsvector.cr +++ b/src/clear/extensions/full_text_searchable/tsvector.cr @@ -82,7 +82,7 @@ class Clear::TSVector when Nil return nil else - raise "Cannot convert #{x.class} to TSVector" + raise Clear::ErrorMessages.converter_error(x.class, "TSVector") end end diff --git a/src/clear/migration/manager.cr b/src/clear/migration/manager.cr index 8f384c33f..cc81ca856 100644 --- a/src/clear/migration/manager.cr +++ b/src/clear/migration/manager.cr @@ -10,6 +10,8 @@ require "./migration" # initialization of the Migration Manager. # class Clear::Migration::Manager + include Clear::ErrorMessages + # Used to migrate between metadata version, in case we need it in the future. METADATA_VERSION = "1" @@ -68,7 +70,7 @@ class Clear::Migration::Manager # Apply negative version if version < 0 - raise "Cannot revert HEAD-#{version}, because no migrations are loaded yet." if current_version.nil? + raise no_migration_yet(version) if current_version.nil? if list_of_migrations.size + version <= 0 version = 0 @@ -210,7 +212,7 @@ class Clear::Migration::Manager if @migrations.any? all_migrations = @migrations.map(&.uid) r = all_migrations - all_migrations.uniq - raise "Some migrations UID are not unique and will cause problem (ids listed here): #{r.join(", ")}" if r.any? + raise migration_not_unique(r) unless r.empty? end end @@ -232,25 +234,24 @@ class Clear::Migration::Manager # Fetch the migration instance with the selected number def find(number) number = Int64.new(number) - @migrations.find(&.uid.==(number)) || raise "Migration not found: #{number}" + @migrations.find(&.uid.==(number)) || raise migration_not_found(number) end # Force up a migration; throw error if the migration is already up def up(number : Int64) : Void m = find(number) - if migrations_up.includes?(number) - raise "Migration already up: #{number}" - else - m.apply(Clear::Migration::Direction::UP) - @migrations_up.add(m.uid) - end + + raise migration_already_up if migrations_up.includes?(number) + + m.apply(Clear::Migration::Direction::UP) + @migrations_up.add(m.uid) end # Force down a migration; throw error if the mgiration is already down def down(number : Int64) : Void m = find(number) - raise "Migration already down: #{number}" unless migrations_up.includes?(number) + raise migration_already_down unless migrations_up.includes?(number) m.apply(Clear::Migration::Direction::DOWN) @migrations_up.delete(m.uid) diff --git a/src/clear/migration/migration.cr b/src/clear/migration/migration.cr index 60f109ef2..1a011974d 100644 --- a/src/clear/migration/migration.cr +++ b/src/clear/migration/migration.cr @@ -60,6 +60,7 @@ # ### module Clear::Migration + include Clear::ErrorMessages module Helper; end include Helper @@ -94,11 +95,7 @@ module Clear::Migration elsif filename =~ /^[0-9]+/ filename[/^[0-9]+/] else - raise \ - "I don't know how to order this migration.\n" + - "– Rename your migration class to have the migration UID at the end of the classname\n" + - "– Rename your file to have the migration UID in front of the filename\n" + - "– Override the method uid ! Be sure the number is immutable\n" + raise uid_not_found(self.class.name) end end) end @@ -121,7 +118,7 @@ module Clear::Migration end def irreversible! - raise IrreversibleMigration.new("Migration #{@name} is irreversible!") + raise IrreversibleMigration.new(migration_irreversible(@name)) end # This will apply the migration in a given direction (up or down) diff --git a/src/clear/model/column.cr b/src/clear/model/column.cr index 672b48b76..b2e1df762 100644 --- a/src/clear/model/column.cr +++ b/src/clear/model/column.cr @@ -7,6 +7,8 @@ require "db" # which is not gathered through the query system (uninitialized column). # Or use the `get_def` to get with default value class Clear::Model::Column(T) + include Clear::ErrorMessages + struct UnknownClass end @@ -24,8 +26,9 @@ class Clear::Model::Column(T) end def value : T - raise "You cannot access to the field `#{name}` " + - "because it never has been initialized" unless defined? + unless defined? + raise illegal_setter_access_to_undefined_column(@name) + end @value.as(T) end @@ -50,8 +53,7 @@ class Clear::Model::Column(T) if T.nilable? @value = x.as(T) else - raise "Your field `#{@name}` is declared as `#{T}` but `NULL` value has been found in the database.\n" + - "Maybe declaring it as `#{T}?` would fix the error." if x.nil? + raise null_column_mapping_error(@name, T) if x.nil? @value = x.not_nil! end diff --git a/src/clear/model/model.cr b/src/clear/model/model.cr index 2f0806219..2dec6c18d 100644 --- a/src/clear/model/model.cr +++ b/src/clear/model/model.cr @@ -6,6 +6,7 @@ require "./converter/**" require "./validation/**" module Clear::Model + include Clear::ErrorMessages include Clear::Model::Connection include Clear::Model::HasColumns include Clear::Model::HasHooks @@ -21,7 +22,7 @@ module Clear::Model getter cache : Clear::Model::QueryCache? def pkey - raise "Please implement primary key for `#{self.class.name}`" + raise lack_of_primary_key(self.class.name) end # We use here included for errors purpose. diff --git a/src/clear/model/modules/is_polymorphic.cr b/src/clear/model/modules/is_polymorphic.cr index 130b533b3..2ec643190 100644 --- a/src/clear/model/modules/is_polymorphic.cr +++ b/src/clear/model/modules/is_polymorphic.cr @@ -83,9 +83,9 @@ module Clear::Model::IsPolymorphic {{c.id}}.new(h, cache, persisted, fetch_columns) {% end %} when nil - raise "No data in column #{@through}, impossible to instantiate !" + raise Clear::ErrorMessages.polymorphic_nil(@through) else - raise "Unknown class: #{h[@through]}" + raise Clear::ErrorMessages.polymorphic_unknown_class(h[@through]) end end end diff --git a/src/clear/sql/delete_query.cr b/src/clear/sql/delete_query.cr index f770c5bd4..d34cbe281 100644 --- a/src/clear/sql/delete_query.cr +++ b/src/clear/sql/delete_query.cr @@ -18,7 +18,7 @@ class Clear::SQL::DeleteQuery end def to_sql - raise QueryBuildingError.new("Delete Query must have a `from` clause.") if @from.nil? + raise Clear::ErrorMessages.query_building_error("Delete Query must have a `from` clause.") if @from.nil? ["DELETE FROM #{@from}", print_wheres].compact.join(" ") end end diff --git a/src/clear/sql/fragment/from.cr b/src/clear/sql/fragment/from.cr index af2888272..3578db098 100644 --- a/src/clear/sql/fragment/from.cr +++ b/src/clear/sql/fragment/from.cr @@ -14,10 +14,10 @@ module Clear::SQL when Symbolic [v, @var].compact.join(" AS ") when SQL::SelectBuilder - raise QueryBuildingError.new("Subquery `from` clause must have variable name") if @var.nil? + raise Clear::ErrorMessages.query_building_error("Subquery `from` clause must have variable name") if @var.nil? ["( #{v.to_sql} )", @var].compact.join(" ") else - raise QueryBuildingError.new("Only String and SelectQuery are allowed as column declaration") + raise Clear::ErrorMessages.query_building_error("Only String and SelectQuery objects are allowed as `from` declaration") end end end diff --git a/src/clear/sql/fragment/join.cr b/src/clear/sql/fragment/join.cr index e21a2792d..65d68d43f 100644 --- a/src/clear/sql/fragment/join.cr +++ b/src/clear/sql/fragment/join.cr @@ -14,7 +14,7 @@ module Clear::SQL def initialize(@from, @condition = nil, type : Symbolic = :inner) @type = if type.is_a?(Symbol) - TYPE[type] || raise QueryBuildingError.new("Type of join unknown: `#{type}`") + TYPE[type] || raise Clear::ErrorMessages.query_building_error("Type of join unknown: `#{type}`") else type end diff --git a/src/clear/sql/query/having.cr b/src/clear/sql/query/having.cr index 2dae25254..33359112b 100644 --- a/src/clear/sql/query/having.cr +++ b/src/clear/sql/query/having.cr @@ -39,7 +39,7 @@ module Clear::SQL::Query::Having begin Clear::Expression[parameters[idx += 1]] rescue e : IndexError - raise QueryBuildingError.new(e.message) + raise Clear::ErrorMessages.query_building_error(e.message) end end @@ -52,7 +52,7 @@ module Clear::SQL::Query::Having sym = question_mark[1..-1] Clear::Expression[parameters[sym]] rescue e : KeyError - raise QueryBuildingError.new(e.message) + raise Clear::ErrorMessages.query_building_error(e.message) end end diff --git a/src/clear/sql/query/order_by.cr b/src/clear/sql/query/order_by.cr index 1f97261ad..602d5d247 100644 --- a/src/clear/sql/query/order_by.cr +++ b/src/clear/sql/query/order_by.cr @@ -12,7 +12,7 @@ module Clear::SQL::Query::OrderBy when "ASC" :asc else - raise "Direction for order_by must be asc or desc" + raise Clear::ErrorMessages.order_by_error_invalid_order(str) end end diff --git a/src/clear/sql/query/where.cr b/src/clear/sql/query/where.cr index 08fbc0280..fd0e8d812 100644 --- a/src/clear/sql/query/where.cr +++ b/src/clear/sql/query/where.cr @@ -91,7 +91,7 @@ module Clear::SQL::Query::Where begin Clear::Expression[parameters[idx += 1]] rescue e : IndexError - raise QueryBuildingError.new(e.message) + raise Clear::ErrorMessages.query_building_error(e.message) end end @@ -109,7 +109,7 @@ module Clear::SQL::Query::Where sym = question_mark[1..-1] Clear::Expression[parameters[sym]] rescue e : KeyError - raise QueryBuildingError.new(e.message) + raise Clear::ErrorMessages.query_building_error(e.message) end end diff --git a/src/clear/sql/sql.cr b/src/clear/sql/sql.cr index a6a339826..9b2d98e45 100644 --- a/src/clear/sql/sql.cr +++ b/src/clear/sql/sql.cr @@ -52,8 +52,7 @@ module Clear @@connections = {} of String => DB::Database def self.connection(connection) : DB::Database - @@connections[connection]? || raise "The database connection " + - "`#{connection}` is not initialized" + @@connections[connection]? || raise Clear::ErrorMessages.uninitialized_db_connection(connection) end alias Symbolic = String | Symbol diff --git a/src/clear/sql/update_query.cr b/src/clear/sql/update_query.cr index 40ddb661f..9b8fad278 100644 --- a/src/clear/sql/update_query.cr +++ b/src/clear/sql/update_query.cr @@ -52,7 +52,7 @@ class Clear::SQL::UpdateQuery end def to_sql - raise Clear::SQL::QueryBuildingError.new("Update Query must have a table clause.") if @table.nil? + raise Clear::ErrorMessages.query_building_error("Update Query must have a table clause.") if @table.nil? ["UPDATE #{@table} SET", print_values, print_wheres].compact.join(" ") end end diff --git a/src/clear/version.cr b/src/clear/version.cr index dcf94f24c..3e79d98a8 100644 --- a/src/clear/version.cr +++ b/src/clear/version.cr @@ -1,3 +1,3 @@ module Clear - VERSION = "v0.2" + VERSION = "v0.3" end