[policy] implement policy equality and tostring #569

mikz · 2018-02-01T12:23:16Z

Implement __eq and __tostring metamethods, so
you can compare policies to see if they are the same.

Also rename apicast.policy.policy to correct place: apicast.policy.
Because it is not a policy per se that can be used, but just a "class".

davidor · 2018-02-01T16:19:45Z

t/deprecation-warnings.t

+--- upstream
+  location /{
+    content_by_lua_block {
+      require('cli')


I'm a bit confused bit this change. A policy chain with { "name": "policy.echo" } like we had before should still raise a deprecation error, right?

yes in this PR still, but in will need some touches in #566
This test is about prefixing it with apicast, so this example is more stable than the policies one.

OK. Didn't think about the implications of this in the other PR.

davidor · 2018-02-01T16:26:00Z

spec/policy_spec.lua

+
+      assert.are.not_equal(p1, p2)
+    end)
+  end)


Nitpick, just for completeness, maybe add a test comparing a policy against itself.

Good one :)

davidor · 2018-02-01T16:27:11Z

spec/policy_spec.lua

+    end)
+  end)
+
+  describe('instance equality', function()


I can imagine that module equality is useful for comparing policies and loading them, but what's the use case for comparing policy instances?

It is useful in tests. Like checking that policy is in the chain: https://github.com/3scale/apicast/pull/566/files#diff-b0a6bd0538ae4f76e9bf6a2a045f51beR36

But this means that two instances of the same policy will be considered to be equal even if they have a different configuration. I think this could be misleading.

True, that is a fair point. But how to solve that? I need to somehow test instances and verifying the configuration would need to use deep equality which is not an easy thing in lua.

But maybe we could check the config just with referential equality and that should be enough for cases I need. But there does not have to be common "config" option, so each policy would have to implement own equality based on properties it sets :/
Thats exactly what you said in #569 (comment) :)

You mentioned that this is only useful in tests.
If you only need this for the specific line that you linked here, I think that using 2 asserts to check version and name is better than introducing this 'dangerous' equality function for instances.

If we are interested in having module eq but not instance eq, wouldn't it be enough to delete the __eq entry from this metatable? : https://github.com/3scale/apicast/pull/569/files#diff-b9119890f218a45887b39e830dfc0bb9R43

Yep, but then to compare them we would have to get the metatable and then __index from it to get the module for comparison.

I see that to access the equality method you'll need to do getmetatable(p1.new()).__index._eq. I don't know if there's a better way to do this.

If this is only going to be used in a test, I'd say it's not that bad. I prefer to have that line somewhere in the tests (as long as it's documented) rather than introduce a dangerous equality function in the codebase. But this is a matter of preference.

You are right, there can be some helper function in the test to abstract it away.

Resolved by having .policy property in the metatable. I think that is best of the both worlds. Allows you easily to get the "module" from the instance so it can be compared, but does not rely on existence of __index (that is entirely up the the creator of the policy)

davidor · 2018-02-01T16:27:53Z

spec/policy_spec.lua

+    it('shows name and version', function()
+      local p1 = policy.new('NAME', 'VERSION').new()
+
+      assert.equal('Policy: NAME (VERSION)', tostring(p1))


This looks a bit strange to me because the policy module and the instances have the same string representation.

Yeah, but I have no good ideas how to differentiate them. This is useful when passing them to inspect and there you can kind of see if it is an instance or not.

We could include the config in the instances. Although it might be a very big table depending on the policy.

Hmm now that I think about it, we don't have a self.config for each policy.
I guess that for now this is good enough. As you said, with inspect we'll be able to clearly differentiate them.

mikz · 2018-02-05T16:33:12Z

gateway/src/apicast/policy.lua

@@ -31,7 +40,7 @@ function _M.new(name, version)
        _NAME = name,
        _VERSION = version or '0.0',
    }
-    local mt = { __index = policy }
+    local mt = { __index = policy, __tostring = __tostring, policy = policy }


@davidor solved it by having a property in the metatable. I think that is best of both worlds.

octobot assigned mikz Feb 1, 2018

mikz force-pushed the policy-equality branch 2 times, most recently from cd7f450 to df10bce Compare February 1, 2018 16:02

mikz requested a review from davidor February 1, 2018 16:02

davidor reviewed Feb 1, 2018

View reviewed changes

mikz force-pushed the policy-equality branch 3 times, most recently from 3014c3f to d0d4884 Compare February 5, 2018 16:31

mikz added 2 commits February 5, 2018 17:31

[policy] rename apicast.policy.policy to apicast.policy

d322a91

[policy] implement policy equality and tostring

91d7e7a

mikz force-pushed the policy-equality branch from d0d4884 to 91d7e7a Compare February 5, 2018 16:32

mikz commented Feb 5, 2018

View reviewed changes

mikz requested a review from davidor February 5, 2018 16:39

davidor approved these changes Feb 5, 2018

View reviewed changes

mikz merged commit 6a025b7 into master Feb 5, 2018

mikz deleted the policy-equality branch February 5, 2018 16:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[policy] implement policy equality and tostring #569

[policy] implement policy equality and tostring #569

mikz commented Feb 1, 2018 •

edited

Loading

davidor Feb 1, 2018 •

edited

Loading

mikz Feb 1, 2018

davidor Feb 1, 2018

davidor Feb 1, 2018

mikz Feb 1, 2018

davidor Feb 1, 2018

mikz Feb 1, 2018

davidor Feb 1, 2018

mikz Feb 1, 2018 •

edited

Loading

davidor Feb 1, 2018

davidor Feb 2, 2018

mikz Feb 2, 2018

davidor Feb 2, 2018 •

edited

Loading

mikz Feb 2, 2018

mikz Feb 5, 2018

davidor Feb 1, 2018

mikz Feb 1, 2018

davidor Feb 1, 2018

davidor Feb 1, 2018 •

edited

Loading

mikz Feb 5, 2018

[policy] implement policy equality and tostring #569

[policy] implement policy equality and tostring #569

Conversation

mikz commented Feb 1, 2018 • edited Loading

davidor Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikz Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidor Feb 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidor Feb 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikz commented Feb 1, 2018 •

edited

Loading

davidor Feb 1, 2018 •

edited

Loading

mikz Feb 1, 2018 •

edited

Loading

davidor Feb 2, 2018 •

edited

Loading

davidor Feb 1, 2018 •

edited

Loading