WIP Update for jsonapi-resources v0.10

[lgebhardt]

@valscion This is an early stage proof of concept to work with v0.10 and fix #64

Many specs are failing because the changes in the processor are now calling Resource.records, which applies the scopes and raises the NotImplementedError. The processor changes are using the new resource replacement for the autogenerated methods that were created in earlier versions. This might be desirable as the scope is being applied to even the source records, however it's not compatible with the tests.

JR v0.10 replaced the records_for method and applies joins against the source resource. Since the ActiveRelation still is based on the Source, even when getting related resources Pundit will detect and use the source's scope. To get around this I'm overriding apply_joins and for every PunditScopedResource joined in I'm adding a subquery that applies the appropriate scope and filters the result set.

In the PunditScopedResource there's a large monkey patch to the JoinManager to add some additional tracking info for the joins. This should go in a future JR version and not added to this gem.

[valscion]

Wow thanks! I don't have time right now to dive into these changes but hopefully will in the near future.

Thanks for describing what has changed inside JR and how it affects this gem, I bet it's gonna be useful

[valscion]

I tried checking out this PR for a few minutes, but the gemspec changes done to development dependencies cause my local development environment to bail out as I am not running bundler 2 yet.

[lgebhardt]

@valscion Sorry about that. I'll try to get that changed soon, along with moving the join tracking to a JR branch we can reference to stop the monkey patching.

[valscion]

Thanks. Would also be useful to be able to see the test failures you mention in CI — right now, the development dependency bumps are causing CI to bail out before having a chance of running the specs at all.

[lgebhardt]

@valscion I've moved the join option tracking to a JR branch, which is referenced in the gemfile, and restored the bundler version.

[valscion]

I did some changes to the CI infrastructure so that we get proper CI test runs now here ☺️. I also fixed the code style so that the only reason CI would show a failure would be that some spec failed.

Seems like there's indeed some test failures that need addressing, especially in the included resources specs. You can run a specific test file locally like so:

bundle exec rspec spec/requests/included_resources_spec.rb

And if you want to run it with some specific combination the CI runs, you can add appraisal "VERSION" before rspec, like so:

bundle exec appraisal "rails-5-2 pundit-2" rspec spec/requests/included_resources_spec.rb

The list of available versions can be found with

bundle exec appraisal list

Note that Ruby 2.3 is required to run the Rails 4.2 tests — other versions should run properly with newer Rubies.

[valscion]

Ok now it should be easier to also debug test failures after #134 was done ☺️. Let me know if I can help you in any way.

[valscion]

I don't know why the GitHub checks isn't visible correctly in this PR, but the failing Travis build is here: https://travis-ci.com/venuu/jsonapi-authorization/builds/144045618

[ksengers]

any progress on this branch??

[cloke]

Is there any status on this PR? We are closely coupled to this gem on several projects and now it appears it could be a blocker to possibly using Ruby 3 in the near future. Are there any items that could we help with? I'm happy to get some eyes on it, but just want to avoid duplicated effort.

[nadnoslen]

Thanks @cloke & @Koen03 for posting here.

I too am blocked from some library upgrades (Rails-6.1 & JSONAPI-Resources-0.10) that might be alleviated if we can get this PR passing tests.

I would love to help ... I've grabbed a fork and am taking a look at the failing tests.

[valscion]

Thanks for people offering to take a new stab at this 😊.

I'd be happy to see a PR that gets this gem compatible with JR 0.10.

[valscion]

If there are people also willing to convert the CI from Travis to GitHub actions, it'd be useful, too ☺️. I think Travis is broken nowadays in this repo but haven't had a chance to investigate yet

[cloke]

If there are people also willing to convert the CI from Travis to GitHub actions, it'd be useful, too ☺️. I think Travis is broken nowadays in this repo but haven't had a chance to investigate yet

I added pr #137 for GitHub actions. It introduces a different issue that I am unsure how to address, but the action definitely works in my fork.

[valscion]

Ok we've got GitHub actions working now, thanks to @cloke and @encounter 🎉

Now we'd need help from any interested parties in creating another pull request that updates jsonapi-authorization to be compatible with jsonapi-resources.

[harbirg]

Any updates on this branch - would love to have v0.10 support with this gem.

[valscion]

Any updates on this branch - would love to have v0.10 support with this gem.

Does not seem like it.

A new pull request would be much appreciated to try to tackle v0.10 support again

[marcelolx]

@harbirg @valscion I'll start working on this next month, but it will take some time to understand how the library works and make the changes.

[valscion]

Sounds good to me, thanks @marcelolx!

It would take quite some time for me to understand the changes done in jsonapi-resources v0.10, too, and that's why I have not had the time to do this update myself.

[harbirg]

@marcelolx - any progress on this?

[marcelolx]

@marcelolx - any progress on this?

Hi @harbirg, not much. I didn't have time enough to work on this (I would like to have). I'll try take a time to work on this but I'm not sure when and how much time it will take.

If someone else wants to work on it, go ahead.

[cloke]

We have a branch that is slowly reducing the number of failing tests. https://github.com/crunchybananas/jsonapi-authorization/tree/v0_10

Something that needs clarity is the branch use_records_for_joined_resources in JR that is required to make this work.

[Subtletree]

@cloke Awesome thanks for your work on this!!

There's a small write up on that branch here: cerebris/jsonapi-resources#1307
@lgebhardt Is the idea to eventually merge use_records_for_joined_resources into master?

[codeguru42]

I am working on a project that uses this branch because we need features from JR 0.10.x. I am encountering the following error on some routes:

Internal Server Error: undefined local variable or method `source_klass' for #<JSONAPI::Authorization::AuthorizingProcessor:0x00007fecefafcbc8>
Did you mean?  resource_klass
               @resource_klass /Users/layne.lund/src/libs/jsonapi-authorization/lib/jsonapi/authorization/authorizing_processor.rb:75:in `authorize_show_relationship'
/Users/layne.lund/.rbenv/versions/2.7.3/lib/ruby/gems/2.7.0/gems/activesupport-6.1.4/lib/active_support/callbacks.rb:427:in `block in make_lambda'
...

Relevant code is here:

      def authorize_show_relationship
        parent_resource = @resource_klass.find_by_key(
          params[:parent_key],
          context: context
        )

        relationship = @resource_klass._relationship(params[:relationship_type].to_sym)

        related_resource =
          case relationship
          when JSONAPI::Relationship::ToOne
            resources_from_relationship(source_klass, source_id, relationship.type, context).first  # LINE 75
          when JSONAPI::Relationship::ToMany
            # Do nothing — already covered by policy scopes
          else
            raise "Unexpected relationship type: #{relationship.inspect}"
          end

        parent_record = parent_resource._model
        related_record = related_resource._model unless related_resource.nil?
        authorizer.show_relationship(source_record: parent_record, related_record: related_record)
      end

From what I can tell, source_klass is not available in this scope. Any suggestions about how to fix this?

Edit:

As I dig into this more, I realize what I've given above probably isn't enough to diagnose the problem. I'm brand new to ruby and naively thought it might be as easy as declaring a variable and initializing it correctly. I'm starting to think there's more going on here that won't be solved by such an easy fix.

[valscion]

Yup most likely the fix required is more work. Contributing a compatibility update to this project is still appreciated and the contribution opportunity is open for anyone to grab right now

[Subtletree]

Something that needs clarity is the branch use_records_for_joined_resources in JR that is required to make this work.

Looks like this has now been merged in a separate PR!

cerebris/jsonapi-resources#1381

[valscion]

Cool! I'm not sure if that merged PR has yet been incorporated to any jsonapi-resources release. Can anyone confirm whether that's the case?

[Subtletree]

Looks like it's included since 0.10.6

[valscion]

We never managed to finish this pull request to the standards we had for an authorization tool. In the end, we decided to no longer support this gem. Discussion here:

• jsonapi-authorization is no longer supported #151