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

Added uniqueness validator for models #227

Merged
merged 3 commits into from
Jun 11, 2018
Merged

Added uniqueness validator for models #227

merged 3 commits into from
Jun 11, 2018

Conversation

Thellior
Copy link
Contributor

@Thellior Thellior commented Jun 10, 2018

What is done

  • Added a new macro "validate_uniqueness" with one parameter called "field"
  • Added test to verify behaviour
  • Added documentation

Explaination

The validator add a feature to the validator helpers that verify if the field value is not already in use. The validator will try to fetch one record that has the same field value. If the value does not exist in the database than the validation will succeed. If the value does exist in the database it will fail if the record is not the same as the model that is being updated.

@Blacksmoke16
Copy link
Contributor

Wouldn't this be better handled at the DB level with a UNIQUE constraint on that column?

Copy link
Member

@drujensen drujensen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 small change requested


macro validate_uniqueness(field)
validate {{field}}, "#{{{field}}} should be unique", -> (model: self) do
# If there is no value than ignore this validation
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✂️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will remove the comment


personUniqueness2.errors[0].message.should eq "name should be unique"

# Should be valid because it should not check uniqueness on itself
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make a separate test for this use case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ofcourse

@Thellior
Copy link
Contributor Author

@Blacksmoke16 You could also do that indeed. But in some use cases you want to have an uniqueness validation error like on a sign up page. If you post the form it will give the error already without having the database throwing the error. So you can show the error on the form with the other validation errors if there are any.

@Blacksmoke16
Copy link
Contributor

Blacksmoke16 commented Jun 10, 2018

Take this for example: with a unique constraint on user_name:

  class User < Granite::Base
    adapter mysql
    table_name users

    primary user_id : Int64, auto: false
    field! user_name : String
  end

User.create(user_id: 1.to_i64, user_name: "foo") # Saves successfully, no errors
User.create(user_id: 2.to_i64, user_name: "foo") # Does not save with error

 @errors=
  [#<Granite::Error:0x55b8b0391450
    @field=:base,
    @message="Duplicate entry 'foo' for key 'users_character_name_uindex'">],

Isn't this pretty much what your validate uniqeness does? Or am i missing a specific use case?

@Thellior
Copy link
Contributor Author

Thellior commented Jun 10, 2018

@Blacksmoke16 Yes you are totally right. But if you have a model that you validate. So let's take as an example User.

class User < Granite::Base
  # Adapters
  adapter pg

  # Table
  table_name users

  # Fields
  field username : String
  field password : String
  field full_name : String
  timestamps

  # Validations
  validate_not_blank :username
  validate_not_blank :password
  validate_not_blank :full_name
end

class Controller < Amber::Controller::Base
  def example
     user = User.new params
    
     if user.valid?
        user.save
        
        respond_with {
           json user
        }
      else
       # Do something with the user.errors
       # For example in the view
     end
  end
end

If you now look at the else statement you can do something with the validation errors.
For example you can add the errors messages on an html page underneath the field.
So if an user now is created with empty fields you get the errors messages.

["password must not be blank", "username must not be blank", "full_name must not be blank"]

If the user than fix the error and submit again it's going to get the database constraint error you described in your comment.
So my validator is going to add the error message to the validation errors. Than it's going to be like
["password must not be blank", "username must be unique", "full_name must not be blank"]

So your html error handling can be the same for the uniqueness error instead of having another error handling for the uniqueness constraint.

And in my API i return the errors like this

{
    "message": {
        "password": [
            "password must not be blank"
        ],
        "username": [
            "username must be unique"
        ]
    },
    "code": 400
}

@Blacksmoke16
Copy link
Contributor

Alright yea, makes sense. Good to see someone using my other validation helpers ;)

👍

@Thellior
Copy link
Contributor Author

I updated the README.md with the new validator option. I see that the documentation in the docs folder is not the same as the README.md. Is the documentation i added enought or do i need to do more?

@Thellior
Copy link
Contributor Author

@drujensen Could you rereview please :). I changed the things you mentioned

Copy link
Member

@drujensen drujensen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥇

@drujensen drujensen merged commit ba90030 into amberframework:master Jun 11, 2018
@drujensen
Copy link
Member

@Thellior It's fine for now. @faustinoaq requested we move to gitbooks and I agree. Its something we should do and replace the readme to point to that instead of maintaining two sets of documents.

@robacarp
Copy link
Member

This is a good addition to the validation libraries, but it is simply not a replacement for database constraints, and can never be. It's very easy for a user to cause a race condition and create two records by double-clicking a submit button.

@Thellior
Copy link
Contributor Author

@robacarp I totally agree but i think you always should have database validations and model validations. This is just an addition for nice error handling

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

Successfully merging this pull request may close these issues.

5 participants