New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ActiveModel::Errors#details #18322

Merged
merged 1 commit into from Jan 21, 2015

Conversation

Projects
None yet
10 participants
@morgoth
Member

morgoth commented Jan 4, 2015

To be able to return type of validator, one can now call details
on Errors instance:

class User < ActiveRecord::Base
  validates :name, presence: true
end
user = User.new; user.valid?; user.errors.details
=> {name: [{error: :blank}]}

The first attempt was to return structure similar to messages, like: {name: [:invalid, :blank]}, but there was problem with missing additional options such as :count in some validators.

If the idea is accepted I will add documentation and changelog.

There is ongoing discussion on https://groups.google.com/forum/#!topic/rubyonrails-core/0lxM684Tqas

@rafaelfranca

View changes

Show outdated Hide outdated activemodel/lib/active_model/errors.rb
@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jan 4, 2015

Member

Maybe we should rename this method to raw_values? It doesn't return just the code.

We can also think about this method being a serialisation of the error object. as_hash can be a candidate.

@jeremy @matthewd @dhh WDYT?

Member

rafaelfranca commented Jan 4, 2015

Maybe we should rename this method to raw_values? It doesn't return just the code.

We can also think about this method being a serialisation of the error object. as_hash can be a candidate.

@jeremy @matthewd @dhh WDYT?

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jan 4, 2015

Member

I'm not sure I understand what the count: 5 extra attribute in the test is about. When do you get these extra params?

Member

dhh commented Jan 4, 2015

I'm not sure I understand what the count: 5 extra attribute in the test is about. When do you get these extra params?

@morgoth

This comment has been minimized.

Show comment
Hide comment
@morgoth

morgoth Jan 4, 2015

Member

There are validators that have additional parameter :count that is passed when generating error message. http://guides.rubyonrails.org/i18n.html#error-message-interpolation

If you think that those can be omitted, it would simplify returned structure to {name: [:invalid, :blank]}

Member

morgoth commented Jan 4, 2015

There are validators that have additional parameter :count that is passed when generating error message. http://guides.rubyonrails.org/i18n.html#error-message-interpolation

If you think that those can be omitted, it would simplify returned structure to {name: [:invalid, :blank]}

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jan 4, 2015

Member

I think that structure would be much nicer. And I like model.errors.to_hash to produce this.

On Jan 4, 2015, at 9:11 AM, Wojciech Wnętrzak notifications@github.com wrote:

There are validators that have additional parameter :count that is passed when generating error message. http://guides.rubyonrails.org/i18n.html#error-message-interpolation http://guides.rubyonrails.org/i18n.html#error-message-interpolation
If you think that those can be omitted, it would simplify returned structure to {name: [:invalid, :blank]}


Reply to this email directly or view it on GitHub #18322 (comment).

Member

dhh commented Jan 4, 2015

I think that structure would be much nicer. And I like model.errors.to_hash to produce this.

On Jan 4, 2015, at 9:11 AM, Wojciech Wnętrzak notifications@github.com wrote:

There are validators that have additional parameter :count that is passed when generating error message. http://guides.rubyonrails.org/i18n.html#error-message-interpolation http://guides.rubyonrails.org/i18n.html#error-message-interpolation
If you think that those can be omitted, it would simplify returned structure to {name: [:invalid, :blank]}


Reply to this email directly or view it on GitHub #18322 (comment).

@morgoth

This comment has been minimized.

Show comment
Hide comment
@morgoth

morgoth Jan 4, 2015

Member

There is already implemented to_hash method that returns error messages. What should we do with it?

Actually, when there are no additional attributes like :count we are returning codes now. Maybe slugs method would be better?

Member

morgoth commented Jan 4, 2015

There is already implemented to_hash method that returns error messages. What should we do with it?

Actually, when there are no additional attributes like :count we are returning codes now. Maybe slugs method would be better?

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jan 4, 2015

Member

Hmmm. Not in love with codes either, but can't come up with a better name off the top of my head either.

Member

dhh commented Jan 4, 2015

Hmmm. Not in love with codes either, but can't come up with a better name off the top of my head either.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jan 4, 2015

Member

(Like slugs even less, though)

Member

dhh commented Jan 4, 2015

(Like slugs even less, though)

@morgoth

This comment has been minimized.

Show comment
Hide comment
@morgoth

morgoth Jan 4, 2015

Member

I changed it to flat structure without extra attributes.

Member

morgoth commented Jan 4, 2015

I changed it to flat structure without extra attributes.

@prathamesh-sonpatki

This comment has been minimized.

Show comment
Hide comment
@prathamesh-sonpatki

prathamesh-sonpatki Jan 4, 2015

Member

May be reasons is better than codes. Model.errors.reasons and {name: [{reason: :blank}]} ?

Member

prathamesh-sonpatki commented Jan 4, 2015

May be reasons is better than codes. Model.errors.reasons and {name: [{reason: :blank}]} ?

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jan 4, 2015

Member

Reasons could just as well be a full message rather than just a symbol. I say codes is reasonable in the absence of something better.

On Jan 4, 2015, at 10:07, प्रथमेश notifications@github.com wrote:

May be reasons is better than codes. Model.errors.reasons and {name: [{reason: :blank}]} ?


Reply to this email directly or view it on GitHub.

Member

dhh commented Jan 4, 2015

Reasons could just as well be a full message rather than just a symbol. I say codes is reasonable in the absence of something better.

On Jan 4, 2015, at 10:07, प्रथमेश notifications@github.com wrote:

May be reasons is better than codes. Model.errors.reasons and {name: [{reason: :blank}]} ?


Reply to this email directly or view it on GitHub.

@morgoth

This comment has been minimized.

Show comment
Hide comment
@morgoth

morgoth Jan 4, 2015

Member

I will work on changes in documentation. We should encourage to adding errors via add method which have support for generating messages and codes.
Maybe we could deprecate set and []= methods?

Member

morgoth commented Jan 4, 2015

I will work on changes in documentation. We should encourage to adding errors via add method which have support for generating messages and codes.
Maybe we could deprecate set and []= methods?

@morgoth

This comment has been minimized.

Show comment
Hide comment
@morgoth

morgoth Jan 4, 2015

Member

@zzak is it possible to document attr_reader methods using RDoc?
Currently there is attr_reader :messages on ActiveModel::Errors which doesn't have documentation and it doesn't show up on docs as callable method.

Member

morgoth commented Jan 4, 2015

@zzak is it possible to document attr_reader methods using RDoc?
Currently there is attr_reader :messages on ActiveModel::Errors which doesn't have documentation and it doesn't show up on docs as callable method.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jan 4, 2015

Member

For what I got from the original discussion is that this feature is to make possible to return these codes as API response and the clients are responsible to show its messages to the users.

But just returning the code would not make this behavior possible for error messages that needs dynamic values, this is why the count: 5 extra attribute were in the tests. Without these attribute the error messages would be only static.

If I got the discussion correctly, we need to think a way to return these attributes.

Member

rafaelfranca commented Jan 4, 2015

For what I got from the original discussion is that this feature is to make possible to return these codes as API response and the clients are responsible to show its messages to the users.

But just returning the code would not make this behavior possible for error messages that needs dynamic values, this is why the count: 5 extra attribute were in the tests. Without these attribute the error messages would be only static.

If I got the discussion correctly, we need to think a way to return these attributes.

@matthewd

This comment has been minimized.

Show comment
Hide comment
@matthewd

matthewd Jan 5, 2015

Member

Yeah, I think the deeply nested hashes are necessary for this to be useful. I suggest details as the method name, and error as the key name.

AFAICS, none of the to_sym calls should be here.

Member

matthewd commented Jan 5, 2015

Yeah, I think the deeply nested hashes are necessary for this to be useful. I suggest details as the method name, and error as the key name.

AFAICS, none of the to_sym calls should be here.

@szimek

This comment has been minimized.

Show comment
Hide comment
@szimek

szimek Jan 5, 2015

Contributor

I'd like just to confirm what @rafaelfranca and @matthewd said - deeply nested hashes are necessary, as otherwise it won't we possible to display dynamic messages like: "Password needs to have at least 8 characters". The list of all built-in validations that use count for interpolation is listed here: http://guides.rubyonrails.org/i18n.html#error-message-interpolation. Additionally, it would be useful for custom date/time validations where it might be necessary to pass a date as interpolation parameter as well.

Contributor

szimek commented Jan 5, 2015

I'd like just to confirm what @rafaelfranca and @matthewd said - deeply nested hashes are necessary, as otherwise it won't we possible to display dynamic messages like: "Password needs to have at least 8 characters". The list of all built-in validations that use count for interpolation is listed here: http://guides.rubyonrails.org/i18n.html#error-message-interpolation. Additionally, it would be useful for custom date/time validations where it might be necessary to pass a date as interpolation parameter as well.

@morgoth

This comment has been minimized.

Show comment
Hide comment
@morgoth

morgoth Jan 5, 2015

Member

I added back deep hash structure with custom options.
I also removed normalizing code to symbol as @matthewd suggested and moved calling Proc to not call it twice (in normalize_message and normalize_code).
Any new ideas about naming or should we go with details and error key?

Member

morgoth commented Jan 5, 2015

I added back deep hash structure with custom options.
I also removed normalizing code to symbol as @matthewd suggested and moved calling Proc to not call it twice (in normalize_message and normalize_code).
Any new ideas about naming or should we go with details and error key?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jan 5, 2015

Member

details seems good to me.

Member

rafaelfranca commented Jan 5, 2015

details seems good to me.

@morgoth

This comment has been minimized.

Show comment
Hide comment
@morgoth

morgoth Jan 5, 2015

Member

Additionally I think we could change initializing @messages in the same way Hash.new { |messages, attribute| messages[attribute] = [] }
Currently this behaviour is weird:

user = User.new
user.errors.get(:email) #=> nil
user.errors[:email] #=> []
user.errors.get(:email) => []
Member

morgoth commented Jan 5, 2015

Additionally I think we could change initializing @messages in the same way Hash.new { |messages, attribute| messages[attribute] = [] }
Currently this behaviour is weird:

user = User.new
user.errors.get(:email) #=> nil
user.errors[:email] #=> []
user.errors.get(:email) => []
@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jan 5, 2015

Member

@morgoth is worth to try, but I think this is a separated PR.

Member

rafaelfranca commented Jan 5, 2015

@morgoth is worth to try, but I think this is a separated PR.

@morgoth

This comment has been minimized.

Show comment
Hide comment
@morgoth

morgoth Jan 5, 2015

Member

sure, I will try to do some refactoring after this is merged

Member

morgoth commented Jan 5, 2015

sure, I will try to do some refactoring after this is merged

@egilburg

This comment has been minimized.

Show comment
Hide comment
@egilburg

egilburg Jan 5, 2015

Contributor

How about one of full_keys, deep_keys, error_keys, nested_keys, etc. I'd prefer using the term keys over codes.

Contributor

egilburg commented Jan 5, 2015

How about one of full_keys, deep_keys, error_keys, nested_keys, etc. I'd prefer using the term keys over codes.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jan 5, 2015

Member

I think keys is too close to Hash#keys which is not what we’re returning. I think “error code” is a pretty well-established term that we’d do well to follow in this case.

On Jan 5, 2015, at 11:04, Eugene Gilburg notifications@github.com wrote:

How about one of full_keys, deep_keys, error_keys, nested_keys, etc. I'd prefer using the term keys over `codes.


Reply to this email directly or view it on GitHub.

Member

dhh commented Jan 5, 2015

I think keys is too close to Hash#keys which is not what we’re returning. I think “error code” is a pretty well-established term that we’d do well to follow in this case.

On Jan 5, 2015, at 11:04, Eugene Gilburg notifications@github.com wrote:

How about one of full_keys, deep_keys, error_keys, nested_keys, etc. I'd prefer using the term keys over `codes.


Reply to this email directly or view it on GitHub.

@egilburg

This comment has been minimized.

Show comment
Hide comment
@egilburg

egilburg Jan 5, 2015

Contributor

What about fields? attributes?

codes reminds me of stuff like HTTP codes like 404, or numeric error codes like -1, not symbols containing field names.

Contributor

egilburg commented Jan 5, 2015

What about fields? attributes?

codes reminds me of stuff like HTTP codes like 404, or numeric error codes like -1, not symbols containing field names.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jan 5, 2015

Member

Both those are used in other contexts too. The only alternative I’ve heard so far that’s worth considering is #symbols, but I don’t think it’s worth the bike-sheeding at this point. #codes is Good Enough.

On Jan 5, 2015, at 14:02, Eugene Gilburg notifications@github.com wrote:

What about fields? attributes?

codes reminds me of stuff like HTTP codes or numeric error codes, not symbols containing field names.


Reply to this email directly or view it on GitHub.

Member

dhh commented Jan 5, 2015

Both those are used in other contexts too. The only alternative I’ve heard so far that’s worth considering is #symbols, but I don’t think it’s worth the bike-sheeding at this point. #codes is Good Enough.

On Jan 5, 2015, at 14:02, Eugene Gilburg notifications@github.com wrote:

What about fields? attributes?

codes reminds me of stuff like HTTP codes or numeric error codes, not symbols containing field names.


Reply to this email directly or view it on GitHub.

@morgoth

This comment has been minimized.

Show comment
Hide comment
@morgoth

morgoth Jan 6, 2015

Member

Can we all agree on one naming? I already changed documentation examples, so it would be the last change in this PR.

Member

morgoth commented Jan 6, 2015

Can we all agree on one naming? I already changed documentation examples, so it would be the last change in this PR.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jan 6, 2015

Member

#codes. Done.

Member

dhh commented Jan 6, 2015

#codes. Done.

@dhh

This comment has been minimized.

Show comment
Hide comment
@dhh

dhh Jan 6, 2015

Member

Actually, fuck it. It doesn't matter. Details is fine. This is not nearly important enough to warrant the back'n'forth.

Member

dhh commented Jan 6, 2015

Actually, fuck it. It doesn't matter. Details is fine. This is not nearly important enough to warrant the back'n'forth.

@morgoth

This comment has been minimized.

Show comment
Hide comment
@morgoth

morgoth Jan 11, 2015

Member

I rebased branch with current master. Is there anything I can improve or can it be merged now?

Member

morgoth commented Jan 11, 2015

I rebased branch with current master. Is there anything I can improve or can it be merged now?

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jan 20, 2015

Member

@morgoth what happen now if people do?

errors.add(:name, "can't be blank")
Member

rafaelfranca commented Jan 20, 2015

@morgoth what happen now if people do?

errors.add(:name, "can't be blank")

@rafaelfranca rafaelfranca added this to the 5.0.0 milestone Jan 20, 2015

@morgoth

This comment has been minimized.

Show comment
Hide comment
@morgoth

morgoth Jan 20, 2015

Member

@rafaelfranca it will work in the same way as currently, but the code hash will look like: {name: [{error: "can't be blank"}]}, so it's not very useful in this context.
If developer wants to use details/codes he need to be aware of that and do something like errors.add(:name, :blank, message: "can't be blank"). I will improve guides about it when we know what final naming is chosen.
This integrates very well with built in validators, where detail/codes works out of the box.

Member

morgoth commented Jan 20, 2015

@rafaelfranca it will work in the same way as currently, but the code hash will look like: {name: [{error: "can't be blank"}]}, so it's not very useful in this context.
If developer wants to use details/codes he need to be aware of that and do something like errors.add(:name, :blank, message: "can't be blank"). I will improve guides about it when we know what final naming is chosen.
This integrates very well with built in validators, where detail/codes works out of the box.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Jan 20, 2015

Member

The patch seems good. Lets change the guides about it in the same PR.

Member

rafaelfranca commented Jan 20, 2015

The patch seems good. Lets change the guides about it in the same PR.

Add ActiveModel::Errors#details
To be able to return type of validator, one can now call `details`
on Errors instance:

```ruby
class User < ActiveRecord::Base
  validates :name, presence: true
end
```

```ruby
user = User.new; user.valid?; user.errors.details
=> {name: [{error: :blank}]}
```
@morgoth

This comment has been minimized.

Show comment
Hide comment
@morgoth

morgoth Jan 20, 2015

Member

@rafaelfranca I added information about using errors.details to guides

Member

morgoth commented Jan 20, 2015

@rafaelfranca I added information about using errors.details to guides

rafaelfranca added a commit that referenced this pull request Jan 21, 2015

@rafaelfranca rafaelfranca merged commit 14599a5 into rails:master Jan 21, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@morgoth morgoth deleted the morgoth:add-error-codes branch Jan 21, 2015

@Bartuz

This comment has been minimized.

Show comment
Hide comment
@Bartuz

Bartuz Oct 10, 2015

Contributor

I love it! Thanks

Contributor

Bartuz commented on cb74473 Oct 10, 2015

I love it! Thanks

This comment has been minimized.

Show comment
Hide comment
@jvenezia

jvenezia May 25, 2016

Awesome! Thanks!

jvenezia replied May 25, 2016

Awesome! Thanks!

@marczking

This comment has been minimized.

Show comment
Hide comment
@marczking

marczking Nov 13, 2015

will this be included in an upcoming rails 4 version aswell or only in rails 5?

marczking commented Nov 13, 2015

will this be included in an upcoming rails 4 version aswell or only in rails 5?

@morgoth

This comment has been minimized.

Show comment
Hide comment
@morgoth

morgoth Nov 14, 2015

Member

@marczking It will be in Rails 5 only. In the meantime you can use https://github.com/cowbell/active_model-errors_details which backports this feature

Member

morgoth commented Nov 14, 2015

@marczking It will be in Rails 5 only. In the meantime you can use https://github.com/cowbell/active_model-errors_details which backports this feature

@marczking

This comment has been minimized.

Show comment
Hide comment
@marczking

marczking Nov 15, 2015

thanks @morgoth , this is great :)

marczking commented Nov 15, 2015

thanks @morgoth , this is great :)

@rafaelfranca rafaelfranca modified the milestones: 5.0.0 [temp], 5.0.0 Dec 30, 2015

vipulnsward added a commit to vipulnsward/rails that referenced this pull request May 1, 2016

Followup of #18322
Mirror the documented new behavior of including details, when performing errors test.

@morgoth morgoth changed the title from Add ActiveModel::Errors#codes to Add ActiveModel::Errors#details May 4, 2016

rafaelfranca added a commit that referenced this pull request May 5, 2016

@Bartuz

This comment has been minimized.

Show comment
Hide comment
@Bartuz

Bartuz Oct 10, 2015

Contributor

I love it! Thanks

Contributor

Bartuz commented on cb74473 Oct 10, 2015

I love it! Thanks

This comment has been minimized.

Show comment
Hide comment
@jvenezia

jvenezia May 25, 2016

Awesome! Thanks!

jvenezia replied May 25, 2016

Awesome! Thanks!

@jvenezia

This comment has been minimized.

Show comment
Hide comment
@jvenezia

jvenezia May 27, 2016

Thanks this is awesome !

I think details could contain more information, like a fully detailed object of what happened.
What misses me today is the message, and details on why there is an error.

class File < ActiveRecord::Base
  validates_inclusion_of :format, in: %w(jpg png)
  validates_length_of :name, maximum: 10
end

file = File.create(format: 'pdf', name: 'Too long file name')
file.errors.details[:format]
# => {error: 'inclusion', value: 'pdf', in: ['jpeg', 'png'], message: 'is not included in the list'}
file.errors.details[:name]
# => {error: 'too_long', count: 18, maximum: 10, message: 'is too long (maximum is 10 characters)'}

This would really help on making simple errors for apis:

For example with jbuilder:

json.error :validation_failed
json.message 'The record is not valid'
json.errors file.errors.details

Would render:

{
  "error": "validation_failed",
  "message": "The record is not valid",
  "errors": {
    "format": [
      {
        "error": "inclusion",
        "value": "pdf",
        "in": ["jpeg", "png"],
        "message": "is not included in the list"
      },
    ],
    "name": [
      {
        "error": "too_long",
        "count": 18,
        "maximum": 10,
        "message": "is too long (maximum is 10 characters)"
      }
    ]
  }
}

For now I have to do this manually.

What do you think?

jvenezia commented May 27, 2016

Thanks this is awesome !

I think details could contain more information, like a fully detailed object of what happened.
What misses me today is the message, and details on why there is an error.

class File < ActiveRecord::Base
  validates_inclusion_of :format, in: %w(jpg png)
  validates_length_of :name, maximum: 10
end

file = File.create(format: 'pdf', name: 'Too long file name')
file.errors.details[:format]
# => {error: 'inclusion', value: 'pdf', in: ['jpeg', 'png'], message: 'is not included in the list'}
file.errors.details[:name]
# => {error: 'too_long', count: 18, maximum: 10, message: 'is too long (maximum is 10 characters)'}

This would really help on making simple errors for apis:

For example with jbuilder:

json.error :validation_failed
json.message 'The record is not valid'
json.errors file.errors.details

Would render:

{
  "error": "validation_failed",
  "message": "The record is not valid",
  "errors": {
    "format": [
      {
        "error": "inclusion",
        "value": "pdf",
        "in": ["jpeg", "png"],
        "message": "is not included in the list"
      },
    ],
    "name": [
      {
        "error": "too_long",
        "count": 18,
        "maximum": 10,
        "message": "is too long (maximum is 10 characters)"
      }
    ]
  }
}

For now I have to do this manually.

What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment