data filtering for rails backend

[cofinalsubnets]

No description provided.

[gkaemmer]

Did it not work with resource = role.org?

[cofinalsubnets]

no, somehow it hit one of the unimplemented paths in the core

[samscott89]

Have you created a task for this?

[cofinalsubnets]

I will once I find a minimal repro 👍

[gj]

Did this happen?

[gj]

ping @GW000

[gkaemmer]

Any way to build these relationships automatically from the activerecord associations? 😬

[gkaemmer]

Totally fine if that's out of scope, just kind of curious

[cofinalsubnets]

that sure would be nice, this is a lot of boilerplate!

[cofinalsubnets]

this is the only place we actually use data filtering

[samscott89]

We should add it to the repos index too

[cofinalsubnets]

the repos index is populated based on org ownership though, not user permissions.

[cofinalsubnets]

I'll add another view I guess that shows all the repos for the current user

[samscott89]

That's probably worth doing anyway -- it's a good way of showing how data filtering is doing something more than a simple filter.

But even if repos are filtered by org, you might want to apply data filtering. For example, in GitHub you have public + private org repos that you can see or not depending on whether you're a member of the org.
That's not the case in the current gitclub, but is rationale for why you would still apply authorization to the org_repos index.

[cofinalsubnets]

Ok the new view is in, it's a good example for data filtering because it shows repos from any org. As it is only org members can view the org repos index so the authorization is already implicit there. The current data filtering API isn't a good fit here unfortunately, you'd need to get the repos the user can read, then filter it again to select the ones belonging to the org.

[cofinalsubnets]

I reflexively pressed the big green button and am not qualified to review my own code 👍

[cofinalsubnets]

this is so action can be a symbol, otherwise the auth just fails

[gkaemmer]

This looks good, my only feedback would be to add longer names and maybe some comments to fetcher.rb -- my guess is we'll be pointing a lot of people towards that file as a way to teach them the concepts

[gkaemmer]

Feeling like this file might be sacrificing some readability for conciseness/performance. IMO the goal here is that someone can look at this file and use it to build their own fetchers, so it's probably worth lengthening variable names, adding some comments, etc.

I took a stab at restructuring it with some longer variable names and inlining the instance_variable_set. What do you think?

module Fetcher
  def self.included(base) # rubocop:disable Metrics/MethodLength, Metrics/AbcSize
    base.class_eval do
      to_activerecord_clause = lambda do |constraint|
        if constraint.field.nil?
          # When field is empty, we should match using ID
          { primary_key => constraint.value.send(primary_key) }
        else
          { c.field => c.value }
        end
      end

      @constraint_handlers = {
        'Eq': ->(query, constraint) { query.where to_activerecord_clause[constraint] },
        'In': ->(query, constraint) { query.where to_activerecord_clause[constraint] },
        'Neq': ->(query, constraint) { query.where.not to_activerecord_clause[constraint] }
      }
      @constraint_handlers.default_proc = proc { |k| "Unsupported constraint kind: #{k}" }
      @constraint_handlers.freeze

      def self.build_query(constraints)
        constraints.reduce(all) do |query, c|
          @constraint_handlers[constraint.kind][query, constraint]
        end
      end

      def self.exec_query(query)
        query.distinct.to_a
      end

      def self.combine_query(one, two)
        one.or(two)
      end
    end
  end

[cofinalsubnets]

You're totally right, I'll change this 👍

[gkaemmer]

Nice

[gj]

@GW000 looks like #15 is redundant after f4d7b94