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

Prevent modification of attribute defaults #175

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mblythe86
Copy link

Description

Either freeze the default attribute values (in the case of String or empty Hash/Array) or call a Proc to return a unique instance of a mutable object (mostly relevant for the principals attribute of PermissionTarget). Add a check in the attribute method to enforce this.

Receiving a FrozenError in this case may be unexpected for users, but it will force them into the preferred method of setting attributes (i.e. using the #{name}= setter).

Related Issue

Fixes issue #174.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change) (This possibly breaks any code that doesn't use the #{name}= setters and relies on this undocumented behavior.)
  • Chore (non-breaking change that does not add functionality or fix an issue)

Checklist:

  • I have read the CONTRIBUTING document.
  • I have run the pre-merge tests locally and they pass.
  • I have updated the documentation accordingly. (I don't think any documentation needs to be updated?)
  • I have added tests to cover my changes.
  • If Gemfile.lock has changed, I have used --conservative to do it and included the full output in the Description above. (No changes to Gemfile.lock)
  • All new and existing tests passed.
  • All commits have been signed-off for the Developer Certificate of Origin.

Either freeze them, in the case of Strings or empty Hash/Array,
or call a proc to return a unique instance of a mutable object
(mostly relevant for the 'principals' attribute of PermissionTarget).

Receiving a FrozenError in this case may be unexpected for users,
but it will force them into the preferred method of setting attributes
(i.e. using the "#{name}=" setter).

Fixes issue chef#174.

Signed-off-by: Matthew Blythe <mblythester+git@gmail.com>
@@ -95,24 +95,24 @@ def from_hash(hash, options = {})
end

# Based on https://github.com/JFrogDev/build-info/blob/master/README.md#build-info-json-format
attribute :properties, {}
attribute :properties, {}.freeze
Copy link
Author

Choose a reason for hiding this comment

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

Why freeze these instead of creating a Proc that would return an empty Hash?

Because the Proc approach could silently drop data. For example, if a user did this:

build = Artifactory::Resource::Build.new
build.properties['key'] = 'value'
puts build.properties

It would print {} instead of {"key" => "value} like a user would expect. This happens because the user modified the mutable default returned by the getter, not an object stored in the object's attribute hash.

If this method of accessing and modifying attributes is common, then the getter could be enhanced to store that default value into the object's attribute hash before returning it.

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.

1 participant