Move Definition functions defined in before_compile to using as overridable

[bitwalker]

The code generated in Definition includes default implementations of
functions which are supposed to be overridable. The problem is that if
the overridable implementations do pattern matching in the function head
and those patterns fail, instead of an error being raised, the default
implementation generated by Waffle is used, which hides the error and
can result in unexpected behavior (in my case, it was the ACL being
incorrectly set).

This change makes those functions overridable like the others, which
preserves the default behavior, but behaves as expected when a custom
implementation is provided.

NOTE: As far as I can tell, this change should preserve the existing behavior, but prevent the bug I described above, but admittedly I may be missing something; let me know if we need to make any adjustments here.

[achempion]

Thank you for the PR, looks good. I'll merge it within couple of days.

[achempion]

Hi,

Thank you for your effort. I looked into PR. It'll introduce breaking changes for most projects using this library who relies on current behaviour and it fails most of the S3 uploading tests. How to run tests with S3.

Acl behaviour is expected because we have @acl attribute which defines default behaviour. You can override default behaviour using your custom implemented functions and if it couldn't match, the @acl attribute will be used.

With proposed changes if we implement acl function, the @acl attribute wouldn't be used as fall back and I think it may confuse some users of this library.

Feedback if welcome. Probably we could change other functions who don't rely on default fall back behaviour.

[bitwalker]

Thanks for taking a look!

Just to be clear, we're running a fork of this library with this patch in production (though it is not fully caught up with master now, since the fork was spun off at the time I created this PR), and have for some time now, with no issues - other than potential changes in master, perhaps something about how we define our storage modules differs from the tests.

Acl behaviour is expected because we have @acl attribute which defines default behaviour.

The fallback behavior with @acl makes sense, and that's why the default implementation in this patch defines the acl/2 function as returning the @acl attribute - this preserves the ability for custom implementations to fallback to the default ACL. You can use super(args) to call the default implementation from your own acl/2 definition, or reference @acl directly.

You can override default behaviour using your custom implemented functions and if it couldn't match, the @acl attribute will be used.

That's not actually the case, and is what this PR is intended to allow you to do. Currently you can't customize the behavior of acl/2 at runtime, because the acl/2 function is not overridable - the before_compile callback is essentially injecting a final definition of that function. But perhaps I'm missing something - if there is another way to override that default at runtime, could you point me to it?

With proposed changes if we implement acl function, the @acl attribute wouldn't be used as fall back and I think it may confuse some users of this library.

Could you clarify why you think this is the case? It wasn't possible previously to define a custom acl/2 function, because the definitions would conflict with the one injected by before_compile, so the only way to customize ACLs up until now was by defining the @acl attribute, but that was relying on a pretty magical set of circumstances that honestly seems a lot more confusing than having users feed the default ACL to use Waffle.Definition, default_acl: :private, and having the default implementation of acl/2 return that value.

Ultimately though, the problem I'm trying to solve here is that acl/2 cannot currently be customized at runtime, unless I've missed something. I'm certainly open to alternative solutions, but this seems like the cleanest one, even though it may impose a small, but nevertheless breaking change on users of the library. Its worth noting that the documentation says you can override acl/2, but that is incorrect (or at least, it was when I created this PR), and fixing that will require a breaking change due to the reliance on @acl being defined before the functions injected by before_compile are defined.

[achempion]

Ultimately though, the problem I'm trying to solve here is that acl/2 cannot currently be customized at runtime

Here is the test case for custom rules and here is the custom rules.

The current implementation generates warnings though and probably we could rewrite it to fix warnings.