Replace caps.Type struct with an interface #322

Merged
merged 4 commits into from Jan 14, 2016

Conversation

Projects
None yet
4 participants
Contributor

zyga commented Jan 13, 2016

The capability type is now an interface. This allows each type to freely
define appropriate logic in a non-declarative way. Some small changes
related to that are the change of the Validate() method to Sanitize()
which can also explicitly mutate a capability (currently this is not
used yet).

Along with this change, caps.Capability.Type is replaced with .TypeName
which is simply a string so that capabilities can be serialized easily.

Signed-off-by: Zygmunt Krynicki zygmunt.krynicki@canonical.com

Replace caps.Type struct with an interface
The capability type is now an interface. This allows each type to freely
define appropriate logic in a non-declarative way. Some small changes
related to that are the change of the Validate() method to Sanitize()
which can also explicitly mutate a capability (currently this is not
used yet).

Along with this change, caps.Capability.Type is replaced with .TypeName
which is simply a string so that capabilities can be serialized easily.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
caps/misc_test.go
@@ -42,8 +40,9 @@ func (s *MiscSuite) TestLoadBuiltInTypes(c *C) {
repo := NewRepository()
err := LoadBuiltInTypes(repo)
c.Assert(err, IsNil)
- c.Assert(repo.types, testutil.Contains, BoolFileType)
- c.Assert(repo.types, HasLen, 1) // Update this whenever new built-in type is added
+ c.Assert(repo.types, DeepEquals, []Type{
@niemeyer

niemeyer Jan 13, 2016

Contributor

For the record, I generally try to avoid such whitebox tests. There's really not much benefit in simply knowing something is inside a given attribute. The best kind of test is those that verify that the value interface behaves as it should. In other words, what is the observable result of this repository knowing about the bool-file type? This outcome cannot change because it's a promise we're making to clients of this API, so it's a better thing to ensure in a test too. The internals can be modified as long as this promise isn't broken.

@zyga

zyga Jan 13, 2016

Contributor

Would this be better?

c.Assert(repo.Type("bool-file"), Not(IsNil))
@zyga

zyga Jan 13, 2016

Contributor

Thanks, I agree about the internal-vs-contract point. I've tweaked the test to check what is really relevant -- that bool-file type is registered.

caps/types.go
- // capability of this type.
- RequiredAttrs []string
+type Type interface {
+ fmt.Stringer
@niemeyer

niemeyer Jan 13, 2016

Contributor

Can we avoid having this here? It just maps to Name.

@zyga

zyga Jan 13, 2016

Contributor

Yeah, I think I just copied that over from older Type. I'll remove it.

@zyga

zyga Jan 13, 2016

Contributor

Done

caps/types.go
- return json.Marshal(t.Name)
+// MockType is a type for various kind of tests.
+// It is public so that it can be consumed from other packages.
+type MockType struct {
@niemeyer

niemeyer Jan 13, 2016

Contributor

s/Mock/Test/? Mock tends to imply a certain kind of behavior from the value.

@zyga

zyga Jan 13, 2016

Contributor

Done

Contributor

niemeyer commented Jan 13, 2016

A few comments above, but LGTM!

zyga added some commits Jan 13, 2016

Remove fmt.Stringer from caps.Type
Since Name() and String() did exactly the same thing, let's just one.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Tweak tests ensuring bool-file is loaded
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Rename caps.MockType to .TestType
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
+ // Unique and public name of this type.
+ Name() string
+ // Sanitize a capability (altering if necessary).
+ Sanitize(c *Capability) error
@mvo5

mvo5 Jan 14, 2016

Collaborator

I'm curious, why was the name changed to Sanitize? That is something you usually do on a toilet to clean it (and in the hospital of course). I'm not a native speaker so I could be wrong and this is bike-shed territory for sure. What exactly will this function do? What does "altering if necessary" means? What kind of alteration?

@zyga

zyga Jan 14, 2016

Contributor

The name changed because the method can now modify the capability and we wanted to make it explicit. I'm open to a better name, we picked sanitize in a quick discussion.

Edit: modify == modify attributes.

@pedronis

pedronis Jan 14, 2016

Contributor

@zyga @mvo5 I have seen "sanitize" used but usually it has a connotation of also making something potentially unsafe safe, (like taking html text and removing dangerous tags), is that the case here? otherwise is a strange term here

@zyga

zyga Jan 14, 2016

Contributor

@pedronis yes, we explicitly reserve the right to modify capability data or reject it entirely.
Sanitize() == Validate() + Alter()

@pedronis

pedronis Jan 14, 2016

Contributor

@zyga that's the weird bit, "sanitize" implies that you almost never reject but make innocuous

@mvo5

mvo5 Jan 14, 2016

Collaborator

I don't know what "Alter()" will do so I don't really know what to suggest. Its just a name, if there is no better idea lets use, we can always change it later :)

Collaborator

mvo5 commented Jan 14, 2016

Code looks good, just one question about the terminology. I'm not hung up on it so no blocker. I just wonder if there is something more fitting. Part of the problem is that I don't know enough about the future, i.e. what "altering the capability" means.

@zyga zyga referenced this pull request Jan 14, 2016

Merged

Bool file sanitization #324

zyga added a commit that referenced this pull request Jan 14, 2016

Merge pull request #322 from zyga/type-iface
Replace caps.Type struct with an interface

@zyga zyga merged commit a6e683c into snapcore:master Jan 14, 2016

2 of 3 checks passed

Integration tests No test results found.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.05%) to 67.533%
Details

@zyga zyga deleted the zyga:type-iface branch Feb 1, 2016

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