Adding transaction raise methods #232

Thellior · 2018-06-17T07:19:18Z

What is done

Added "helper" methods that raises exceptions on errors. The idea is that sometimes you want to be sure that an transaction method has succeeded. If the method did not complete successfully than it will raise an exception.

Methods

Added update method that accepts args that will set the attributes of the model and tries to save it
Added update! method that accepts args that will set the attribute of the model and tries to save! it
Added save! method that will call the save method and raises an exception if it failed
Added create! method that will call the create method and raises an exception if it failed
Added destroy! method that will call the destroy method and raises an exception if it failed
Added custom exceptions for the methods so an application developer is able to catch specific errors

eliasjpr

A couple of small stylistic comments. Thanks for the work on this.

eliasjpr · 2018-06-17T11:14:57Z

src/granite/transactions.cr

@@ -1,4 +1,6 @@
 module Granite::Transactions
+  class TransactionFailed < Exception; end


TransactionError instead

eliasjpr · 2018-06-17T11:17:19Z

src/granite/transactions.cr

+    def create!(args : Hash(Symbol | String, DB::Any) | JSON::Any)
+      instance = create(args)
+
+      raise Granite::Transactions::TransactionFailed.new("Could not create #{self.name}") unless instance.errors.empty?


This line is very long also I dont think you need to specify Granite::Transactions::

Yeah, looks like Java 😅

Ok, i worked on validation helper last time and all methods were one liners so i removed my multiline if statement for this one. But i will change it. I will remove the prefix module names

Blacksmoke16

Just one thing i noticed.

Blacksmoke16 · 2018-06-17T14:24:11Z

src/granite/transactions.cr

@@ -158,6 +179,10 @@ module Granite::Transactions
      end
      true
    end
+
+    def destroy!
+      save || raise Granite::Transactions::TransactionError.new("Could not destroy #{self.name}")


Shouldn't this be destroy || raise Granite::Transactions::TransactionError.new("Could not destroy #{self.name}")

robacarp

Thanks for your contribution, and especially for writing nice tests around the added functionality.

I left a couple style comments but the custom exception gives me pause.

robacarp · 2018-06-17T15:09:35Z

src/granite/transactions.cr

+    def create!(args : Hash(Symbol | String, DB::Any) | JSON::Any)
+      instance = create(args)
+
+      if !instance.errors.empty?


This would read better as if instance.errors.any?

robacarp · 2018-06-17T15:10:37Z

src/granite/transactions.cr

@@ -1,4 +1,6 @@
 module Granite::Transactions
+  class TransactionError < Exception; end


Do we really want to create a custom exception class for this? I doubt it, but I'd like to hear from @drujensen on this.

If so, it needs to be named differently. This file is called 'transactions' but it doesn't yet do anything with the SQL concept of a transaction. It's not really a problem because the filename isn't presented to an appdev.

This exception will be handled and used directly by an appdev, and the name is misleading.

Do we really want to create a custom exception class for this? I doubt it, but I'd like to hear from @drujensen on this.

@robacarp yeah, I think we do. We have Granite::Error class to handle errors so I think having our own exception makes sense. I would rename this to Granite::Exception. Not sure I would place it here in this file. Maybe in its own file /src/granite/exception.cr?

I went looking for the way ActiveRecord does this and here's what I found.

ActiveRecordError - Generic error class and superclass of all other errors raised by Active Record.

RecordInvalid - raised by ActiveRecord::Base#save! and ActiveRecord::Base.create! when the record is invalid.

RecordNotFound - No record responded to the ActiveRecord::Base.find method. Either the row with the given ID doesn't exist or the row didn't meet the additional restrictions.

StatementInvalid - The database server rejected the SQL statement. The precise error is added in the message.

After thinking on it, as an application developer, when I write the save! method, I want to be able to capture the specific exception that is going to be raised by save! and not other random exceptions.

I retract my statement about removing this custom exception. Instead I think that it should be a Granite::InvalidRecord exception. It's more helpful and explicit, and allows use of the save! / create! methods without opening up to a category of bugs about catching the wrong exception.

( Rails destroy! raises a ActiveRecord::RecordNotDestroyed )

robacarp · 2018-06-17T15:11:42Z

src/granite/transactions.cr

@@ -143,6 +159,11 @@ module Granite::Transactions
      true
    end

+
+    def save!
+      save || raise Granite::Transactions::TransactionError.new("Could not save #{self.name}")


I like this "save || raise" format better than the previous "raise unless save", but as someone else said, the explicit prefix Granite::Transactions can be dropped from these.

robacarp · 2018-06-17T15:17:53Z

spec/granite/transactions/create_spec.cr

+  describe "{{ adapter.id }} .create!" do
+    it "creates a new object" do
+      parent = Parent.create!(name: "Test Parent")
+      parent.id.should_not be_nil


These would read better as parent.persisted.should be_true

resolved

robacarp · 2018-06-18T22:23:23Z

spec/granite/transactions/destroy_spec.cr

@@ -55,5 +55,37 @@ module {{adapter.capitalize.id}}
      end
    end
  end
+
+  class ParentWithCallback < Granite::Base


there's a model in spec/spec_models.cr that I think satisfies this already called CallbackWithAbort

robacarp · 2018-06-18T22:26:46Z

spec/granite/transactions/destroy_spec.cr

+      found.should be_nil
+    end
+
+    it "does not destroy an invalid object but raise an exception" do


I'm not sure the object validity has anything to do with the reason for not being destroyed. It could have already been destroyed, or a callback halted destroy, etc

I agree on that but i could not find an way to raise an database exception to verify this behaviour. Granite will generate the following query "DELETE FROM "parents" WHERE "id"=$1". If the record does not exist it will still run the query and succeed. So i could not raise an "DB::Error" so i was verify the behaviour with an "Granite::Callbacks::Abort" error. I verified that the delete query never runned. I will add an test that verifies that the record is still there

Great explanation. To be clear, I don't have any problem with the way you're triggering the error condition, I was just commenting on the description text of the test itself. :]

robacarp · 2018-06-18T22:29:28Z

src/granite/exceptions.cr

@@ -0,0 +1,13 @@
+module Granite
+  class RecordInvalid < ::Exception


Would this be more grammatically correct as "InvalidRecord"?

I followed the Rails convention you mentioned in the comment. So i introduced RecordNotDestroyed and RecordInvalid into the stack. Do you want me to change it to InvalidRecord instead?

Generally with Amber I'm not as interested in following in Rails' footsteps as I am in producing something that feels natural. Sometimes that means features similar to Rails, sometimes not.

robacarp

This is looking great. I left a few comments, but nothing major. Thank you for adding the exceptions and specs to go along with them.

issues resolved

… one

Thellior · 2018-06-19T20:14:57Z

After discussion with @robacarp on gitter we decided that we are going to change "RecordInvalid" to "RecordNotSaved". We are going to add an model property on the exception so people who are catching the specific exception are having access to the instance.

eliasjpr · 2018-06-19T20:27:38Z

@Thellior great work on this! 🥇

robacarp · 2018-06-19T22:23:52Z

spec/granite/exceptions/record_invalid_spec.cr

-class RecordInvalidParent; end
-class Granite::RecordInvalidParent; end
+{% for adapter in GraniteExample::ADAPTERS %}
+module {{adapter.capitalize.id}}


Thellior · 2018-06-21T06:52:35Z

Can we merge this?

faustinoaq

@robacarp 🔔 😅

@Thellior Thank you! 👍

faustinoaq · 2018-06-21T07:11:16Z

@drujensen Should we release granite v0.12.2 or v0.13.0? 😅

Thellior added 5 commits June 17, 2018 09:13

Added save! method

c59aa5c

Added destroy! method

3e09cac

Added create! method

22ade91

Added nil? check on create!

f5da1f1

Changed nil check to errors check

a2267ce

Thellior changed the title ~~[WIP] Adding transaction raise methods~~ Adding transaction raise methods Jun 17, 2018

faustinoaq added kind:enhancement pr:needs-review kind:feature labels Jun 17, 2018

faustinoaq requested a review from a team June 17, 2018 09:03

eliasjpr previously requested changes Jun 17, 2018

View reviewed changes

Review changes

caaf77b

Blacksmoke16 previously requested changes Jun 17, 2018

View reviewed changes

Update transactions.cr

8db8810

robacarp requested changes Jun 17, 2018

View reviewed changes

Thellior added 4 commits June 18, 2018 21:49

Added custom exceptions and added tests for them

c851203

Removed Nil type because i fixed an bug than produced Nil values

f8e16f6

Added update and update! methods

1d83b77

Changed other spec files with persisted? where possible

75aeaa5

robacarp reviewed Jun 18, 2018

View reviewed changes

Thellior added 3 commits June 19, 2018 19:51

Using callback model in the spec_models instead of introducing an new…

789ade9

… one

Docker does not cleanup callback model properly. abort_at = 'temp' fixes

4952793

Forgot to change to save! again

e6e64c7

Merge branch 'master' into adding-transactions-raise-methods

157eb48

Thellior added 2 commits June 19, 2018 22:44

RecordInvalid to RecordNotSaved and model is now attached to the errors

41dc2a7

Added new methods to the README.md

46ca75c

robacarp reviewed Jun 19, 2018

View reviewed changes

robacarp approved these changes Jun 19, 2018

View reviewed changes

faustinoaq approved these changes Jun 21, 2018

View reviewed changes

robacarp removed kind:enhancement pr:needs-review labels Jun 21, 2018

robacarp merged commit f2feca3 into amberframework:master Jun 21, 2018

Thellior mentioned this pull request Jun 23, 2018

Currently missing save!, destroy!, update! #230

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding transaction raise methods #232

Adding transaction raise methods #232

Thellior commented Jun 17, 2018 •

edited

Loading

eliasjpr left a comment

eliasjpr Jun 17, 2018

eliasjpr Jun 17, 2018

faustinoaq Jun 17, 2018 •

edited

Loading

Thellior Jun 17, 2018

Blacksmoke16 left a comment

Blacksmoke16 Jun 17, 2018

robacarp left a comment

robacarp Jun 17, 2018

robacarp Jun 17, 2018

drujensen Jun 17, 2018

robacarp Jun 17, 2018

robacarp Jun 17, 2018

robacarp Jun 17, 2018

robacarp Jun 17, 2018

robacarp Jun 18, 2018

robacarp Jun 18, 2018

Thellior Jun 19, 2018

robacarp Jun 19, 2018

robacarp Jun 18, 2018

Thellior Jun 19, 2018

robacarp Jun 19, 2018

robacarp left a comment

Thellior commented Jun 19, 2018

eliasjpr commented Jun 19, 2018

robacarp Jun 19, 2018

Thellior commented Jun 21, 2018

faustinoaq left a comment

faustinoaq commented Jun 21, 2018

		@@ -1,4 +1,6 @@
		module Granite::Transactions
		class TransactionFailed < Exception; end

		@@ -1,4 +1,6 @@
		module Granite::Transactions
		class TransactionError < Exception; end

		@@ -0,0 +1,13 @@
		module Granite
		class RecordInvalid < ::Exception

Adding transaction raise methods #232

Adding transaction raise methods #232

Conversation

Thellior commented Jun 17, 2018 • edited Loading

What is done

Methods

eliasjpr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

faustinoaq Jun 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Blacksmoke16 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robacarp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robacarp left a comment

Choose a reason for hiding this comment

Thellior commented Jun 19, 2018

eliasjpr commented Jun 19, 2018

Choose a reason for hiding this comment

Thellior commented Jun 21, 2018

faustinoaq left a comment

Choose a reason for hiding this comment

faustinoaq commented Jun 21, 2018

Thellior commented Jun 17, 2018 •

edited

Loading

faustinoaq Jun 17, 2018 •

edited

Loading