Skip to content
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

validate false positive when property name is Object method #314

Closed
smathy opened this issue Nov 13, 2015 · 13 comments
Closed

validate false positive when property name is Object method #314

smathy opened this issue Nov 13, 2015 · 13 comments

Comments

@smathy
Copy link

smathy commented Nov 13, 2015

Might not be only Object methods, but is definitely at least those:

require "reform/form/active_model/validations"
class BadForm < Reform::Form
  include Reform::Form::ActiveModel::Validations

  property :trust        # or anything from `Object.public_methods`
  validates :trust, presence: true
end

BadForm.new(Struct.new(:trust).new).validate({})
# => true
@apotonick
Copy link
Member

How could we catch that, though??

We can't test if the method is already defined: while this might work in a normal form, in inherited forms, this will create warnings even though nothing is wrong. Any ideas?

@smathy
Copy link
Author

smathy commented Dec 20, 2015

I don't know how you've implemented that stuff, it seems that instead of using some internal structure for the properties (eg. hash keys) that at the time of validation those properties have already been attached to a ruby object as methods, which means there will always be a false positive for any existing Object methods.

I think the validation needs to be done on a separate data structure where the properties are not methods.

@apotonick
Copy link
Member

The validation is done in another object, which is still an object, though! 😬

The problem is when initializing the form, the form asks the model for its properties, e.g. model.trust and that - apparently - returns true?

@smathy
Copy link
Author

smathy commented Dec 20, 2015

Right, maybe it should ask model.attributes or something, because based on what I think you're saying even an AR method, or method in the model itself will cause a false positive

@apotonick
Copy link
Member

Does your model have a trust field? Can you paste results for the following test?

model.trust
YourForm.new(model).trust

@smathy
Copy link
Author

smathy commented Dec 20, 2015

You mean in the original code, or in the reproduction that I included in this issue? Obviously there the model is just Struct.new(:trust).new so your example becomes:

[29] pry(main)> model = Struct.new(:trust).new
=> #<struct  trust=nil>
[30] pry(main)> model.trust
=> nil
[31] pry(main)> BadForm.new(model).trust
=> nil

@smathy
Copy link
Author

smathy commented Dec 20, 2015

...and yet:

[32] pry(main)> BadForm.new(Struct.new(:trust).new).validate({})
=> true

@RKushnir
Copy link

RKushnir commented Jun 8, 2016

I think, the problem essentially boils down to https://github.com/apotonick/reform/blob/79032a3b86378306b3942ab1948e42168697c66c/lib/reform/form/active_model/validations.rb#L68

> model = Struct.new(:trust).new
=> #<struct  trust=nil>
> SimpleDelegator.new(model).trust
=> #<struct  trust=nil>
> model.trust
=> nil

@fran-worley
Copy link
Collaborator

As of Reform 2.2 all active model / active record related code has moved to the reform-rails gem.

If you're still experiencing this issue, please can you reopen it in the correct gem. Thanks 😺

@smathy
Copy link
Author

smathy commented Aug 9, 2016

Hmm, working minimal code showing problem, months pass, then redirect to another gem without even a link. Nope.

If you're interested in seeing if this bug still exists in your gem then paste my sample code into a rails console and you'll see.

@fran-worley
Copy link
Collaborator

Thanks for coming back to me @smathy. The reform- rails gem can be found at https://github.com/trailblazer/reform-rails. I didn't include the link in the first place as its the first and only repo of that name that comes up when you search github for reform-rails.

The main reason I closed this was that there was no update from you since 2015 and the bump in June a) triggered no response from yourself and b) is specific to ActiveModel Validations which has been moved to the new gem as I mentioned.

Are you still using Reform? It would be useful to know if the problem persists in 2.2.

@RKushnir
Copy link

RKushnir commented Aug 9, 2016

I can confirm the issue still exists, here's the new home of the line I mentioned above https://github.com/trailblazer/reform-rails/blob/c0124fccab74668e340f2dbdf0dba0e3d550604b/lib/reform/form/active_model/validations.rb#L67

@fran-worley
Copy link
Collaborator

Thanks @RKushnir I've opened a new issue in reform-rails see trailblazer/reform-rails#17.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants