Remove redundant validates_unique where DB unique index already exists#4930
Remove redundant validates_unique where DB unique index already exists#4930serdarozerr wants to merge 13 commits intocloudfoundry:mainfrom
Conversation
- Model's that already has uniqueness constraint on db level are refactored. sequel validate_unique method removed from list.
…s validation in service_broker_registraion service to refraing http call invalid brokers
…s validation in service_key_manager to refraing http call in service_key_create
|
|
||
| def validate | ||
| validates_presence :name | ||
| validates_unique :name, dataset: Domain.dataset |
There was a problem hiding this comment.
The problem with removing this validation is that now another one is triggering (name_overlaps?) which actually checks something else (e.g. "The domain name "foo.com" cannot be created because "bar.foo.com" is already reserved by another domain") and thus results in a wrong error message. There doesn't seem to be a test that verifies that the index-based validation is failing for domains.
| other_domain.name = subject.name | ||
| expect(other_domain).not_to be_valid | ||
| expect(other_domain.errors[:name]).to include(:unique) | ||
| expect do |
There was a problem hiding this comment.
See comment in app/models/runtime/domain.rb - this now tests something else...
|
|
||
| describe 'Validations' do | ||
| it { is_expected.to validate_presence :name } | ||
| it { is_expected.to validate_uniqueness :name } |
There was a problem hiding this comment.
Here we should add a test for the index-based validation.
| org_create.create(message) | ||
| end.not_to raise_error | ||
|
|
||
| # Mock the validation for the second request to simulate the race condition and trigger a unique constraint violation |
There was a problem hiding this comment.
I think the complete context could be removed, we don't have to test concurrency if the database enforces uniqueness.
| SpaceCreate.new(user_audit_info:).create(org, message) | ||
| end.not_to raise_error | ||
|
|
||
| # Mock the validation for the second request to simulate the race condition and trigger a unique constraint violation |
There was a problem hiding this comment.
I think the complete context could be removed, we don't have to test concurrency if the database enforces uniqueness.
|
|
||
| describe 'Validations' do | ||
| it { is_expected.to validate_presence :guid } | ||
| it { is_expected.to validate_uniqueness :guid } |
There was a problem hiding this comment.
Is uniqueness tested somewhere else?
There was a problem hiding this comment.
Because of same reason that, in action tests, this concurrency test suit already check uniqueness implicitly, i thought adding uniqueness test would be redundant in this case.
| it { is_expected.to validate_presence :broker_url } | ||
| it { is_expected.to validate_presence :auth_username } | ||
| it { is_expected.to validate_presence :auth_password } | ||
| it { is_expected.to validate_uniqueness :name, message: Sequel.lit('Name must be unique') } |
There was a problem hiding this comment.
Is uniqueness tested somewhere else?
There was a problem hiding this comment.
Because of same reason that in action tests, this concurrency test suit already check uniqueness implicitly, i thought adding uniqueness test would be redundant in this case.
| raise e | ||
| end | ||
| self | ||
| rescue Sequel::ValidationFailed |
There was a problem hiding this comment.
Maybe add a comment that "errors have been added"...
| post '/v2/service_brokers', public_body | ||
| expect(last_response).to have_status_code(201) | ||
|
|
||
| stub_catalog |
There was a problem hiding this comment.
Did you understand why this needs to be added? Because I don't...
There was a problem hiding this comment.
This stub added because of the validation in here i assume this validation called just before http call to prevent extra calls if broker is not valid etc.(kind of guard flag) Since we removed uniqueness_validation check, http call hits , so we stub to prevent error throwing.
… tests are reintroduced in unit/models.
3 participants
Models with a DB-level unique index were performing an extra SELECT on every save via Sequel's validates_unique. This PR removes that redundant check and relies on around_save to catch Sequel::UniqueConstraintViolation instead, letting the database enforce what it already enforces.
I have reviewed the contributing guide
I have viewed, signed, and submitted the Contributor License Agreement
I have made this pull request to the
mainbranchI have run all the unit tests using
bundle exec rakeI have run CF Acceptance Tests