Default pundit authorizer keyword args

[nruth]

I saw in #48 that help was wanted using keyword args for JSONAPI::Authorization::DefaultPunditAuthorizer so I've had a go at it. I've done 2 methods to see how it goes, and I'd like feedback to check I'm doing what you wanted before continuing.

The only hard part I found was making the AuthorizationStubs helpers work with keyword args as well as splat args. I'm not entirely happy with how I solved that, so any thoughts on that would be great. To see why I've done that, try changing the stub method and re-run the tests and you'll find a lot of things start breaking in various places with wrong-number-of-argument errors and other fun things.

[valscion]

Hi! Thanks for taking a stab at this ☺️

Looks like the tests are passing right now, nice 👍. Code style linter is causing the CI to be red, but the specs do go through and that's nice. You might want to run bundle exec rubocop locally to see the errors on your machine, and if possible, configure your editor to show those errors inline to make the linter cause less anguish 😅

The AuthorizationStubs are a bit difficult, I agree. I do however think that the solution with .tap is alright, and we can keep using that ☺️. It's too bad that we can't use .to receive(operation).with(*args, **kwargs) even if kwargs were empty.

I wonder if, after converting all authorizer methods to use kwargs, will we even need the "non-kwargs" call anyway? So we might be able to get rid of the .tap-dance (heh) after the refactor is complete? 🎉

[nruth]

Thanks, I'll do that.

I'd like to mention a couple of other things, assuming semantic versioning:

• Renaming: some of the kw-args feel like they could be renamed/shorter. I've not done this initially because I don't feel I have any kind of ownership over the code and it'd make the changes harder to accept. Before 1.0 would however be the best time to consider this, because after that the API is more painful to change. I've not used this library much so I don't know how much changing this api would affect users, or if it's all internal code. I'm assuming it would affect some people who are writing their own authenticators, since the class name has "default" in it?

• Do all of the methods need keyword arugments? The ones with 1 argument and an obvious name will be easier to write with positional args. I personally prefer keyword arguments almost everywhere, and it might make the *args, **kwargs problem go away as you mentioned (depending on which other classes are tested with those helpers, and if they also get changed to kwargs). Making it consistent across all the methods in the class, and making it self-documenting, would probably be good things, but it is more typing or boilerplate – all very stylistic, so up to you really?

I'll continue with the simplest options for now, but as I mentioned it's probably better to get the API right (if that matters? if it's not frequently used maybe it does not) before a major release.

[valscion]

Good question regarding the argument names.

I took a look at the current argument names, and only related_records_with_context in replace_fields and create_resource methods I'm unsure about.

jsonapi-authorization/lib/jsonapi/authorization/default_pundit_authorizer.rb

Line 91 in da04d6c

def replace_fields(source_record, related_records_with_context)

jsonapi-authorization/lib/jsonapi/authorization/default_pundit_authorizer.rb

Line 103 in da04d6c

def create_resource(source_class, related_records_with_context)

I think they'll be quite easy to rename after this PR to something else, so you don't have to worry about that here. That is, unless you get some amazing idea for a better name, then we can discuss it 😄

I'm assuming it would affect some people who are writing their own authenticators, since the class name has "default" in it?

Yeah, that has been the case. We've documented that people can use a different authorizer in the past here: https://github.com/venuu/jsonapi-authorization/blob/v0.8.2/README.md#configuration

I've also mentioned that subclassing DefaultPunditAuthorizer is a way to cater to different requirements, for example here #39 (comment). I think that @arcreative is using a custom authorizer class (seeing as he wrote this issue #77), subclassing DefaultPunditAuthorizer, so he might have insight into what would be good argument names.

---

Do all of the methods need keyword arugments? The ones with 1 argument and an obvious name will be easier to write with positional args.

I think it's for the best to use keyword arguments for all methods. It makes the authorizer class methods consistent, and also makes it clearer when the method is passed a model class compared to model instance. For example:

jsonapi-authorization/lib/jsonapi/authorization/default_pundit_authorizer.rb

Line 30 in da04d6c

def find(source_class)

☝️ the #find method gets a class, and:

jsonapi-authorization/lib/jsonapi/authorization/default_pundit_authorizer.rb

Line 39 in da04d6c

def show(source_record)

☝️ the #show gets a record / instance of a class.

---

I'll continue with the simplest options for now, but as I mentioned it's probably better to get the API right (if that matters? if it's not frequently used maybe it does not) before a major release.

That sounds like a good approach to me! 👍 I don't think many people rely on the exact API of DefaultPunditAuthorizer (at least I hope so 😅) so changes done there shouldn't be an issue for most people.

[nruth]

I agree with all of that. Thanks for the feedback.

[nruth]

I am a bit sad about this change, but I see the benefit of having the style rules enforced automatically & not spending time agonising over lots of custom rules (as I probably now will while adding this setup to my own project).

[valscion]

Yeah, code styles can differ between projects. I'm myself happy about this change, as I prefer ternaries over one line if ... then ... else ... end statements 😛

[nruth]

I think this is finished, but would like your opinion on the unused parameters in include_has_one_resource and include_has_many_resource. Can these be deleted?

Otherwise, we either need to do this (what I've done currently, on 2 methods):

# rubocop:disable Lint/UnusedMethodArgument
def include_has_many_resource(source_record:, record_class:)
  ::Pundit.authorize(user, record_class, 'index?')
end
# rubocop:enable Lint/UnusedMethodArgument

or set AllowUnusedKeywordArguments to true in the rubocop config.

[valscion]

Thanks! Looking forward to looking into this 😊. I'll get to this on Monday most likely

[valscion]

I actually got to looking into this right now 😊.

This looks great to me! I think that it's the best approach to leave the rubocop disable comments in-place, as even if the default authorizer doesn't use those arguments, they can be useful for custom authorizer classes.

It's also possible to add only rubocop:disable TheOffendingRule to the end of a line to disable a rule for only that line. I don't really have a strong feeling one way or the other, so whichever approach suits you the most is fine by me ☺️.

[valscion]

Is this OK for merging, or is there still something you'd like to do?

[nruth]

I think the code is ready but I haven't checked if any of the documentation needs revising.

[valscion]

Yeah might be useful to check if the generated rubydocs still look good. That can happen in another PR as well.

I don't remember off the top of my head that how these rubydocs were generated in the first place 😅

[nruth]

The classname doesn't appear in the project files outside of the file comments and specs, so I assume there isn't other documentation for this.

Upgrading user wise, the runtime documentation (cough error messages cough) should make it clear what to do. So I'm finished on this unless you'd like something else doing.

A nice thing for the changelog / version notes for v1 probably needs writing but I'm hoping you will do that?

[valscion]

Yeah I can handle the changelog ☺️. Thanks a bunch for helping out! I'd like to add you to the contributors section in the README if that's OK to you?

[nruth]

Great, thanks! Yes please do.