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

Raising UnknownAttribute when adding an attribute not in the model #463

Merged
merged 2 commits into from
Jul 22, 2020

Conversation

AlexGascon
Copy link
Contributor

Fixes #462

Changes

Modified the method write_attribute to raise an UnknownAttribute error if the user tries to write a field that hasn't been defined on the model class

Created a new module, Dynamoid::Persistence::UpdateValidations, with a method to validate if a specific attribute is part of a model class

Adding tests to validate the new behavior in:

  • .upsert
  • .update_fields
  • .update
  • #update
  • #update_attribute
  • #update_attributes

Modifying the method `write_attribute` to raise an `UnknownAttribute`
error if the user tries to write a field that hasn't been defined on the
model class

Creating a new module, `Dynamoid::Persistence::UpdateValidations`, with
a method to validate if a specific attribute is part of a model class

Adding tests to validate the new behavior in:
- `.upsert`
- `.update_fields`
- `.update`
- `#update`
- `#update_attribute`
- `#update_attributes`
@@ -497,6 +498,8 @@ def save(options = {})
def update_attributes(attributes)
attributes.each { |attribute, value| write_attribute(attribute, value) }
save
rescue Dynamoid::Errors::UnknownAttribute
false
Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense to follow Rails convention and raise UnknownAttribute exception anyway even in methods without bang.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me, I'll make the change

@andrykonchin
Copy link
Member

Looks pretty good 👍 . There is only one issue to discuss.

@andrykonchin
Copy link
Member

Good job. Thank you for the contribution.

@andrykonchin andrykonchin merged commit 8423724 into Dynamoid:master Jul 22, 2020
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

Successfully merging this pull request may close these issues.

NoMethodError when trying to add an attribute that isn't on the model
2 participants