[WIP] Enhancing sort_by to support sorting on association attributes and count

[abellotti]

Enhancing sort_by to support sorting on single level association attribute.

Usage: sort_by: association.attr[:[asc|desc]]

where the association is the pluralized association, i.e. application_types

Also, allows sorting by the association count.

Usage: sort_by: assocation.@count[:[asc|desc]]

Fix for TPINVTRY-770

[abellotti]

TODO:

• [x] support sorting by association count
• [x] add specs

[abellotti]

Tested examples:

{
  sources(sort_by: ["application_types.display_name:desc", "name:asc"]) {
    id
    name
    application_types {
      id
      name
      display_name
    }
  }
}

{
  sources(sort_by: ["application_types.display_name:asc", "id:desc"]) {
    id
    name
    application_types {
      id
      name
      display_name
    }
  }
}

[abellotti]

Added support for sorting by association count. This is supported via the "association.count[:[asc|desc]]" syntax.

Example:

{
  sources(sort_by: ["application_types.@count:desc", "application_types.display_name:asc"]) {
    id
    name
    application_types {
      display_name
    }
  }
}

[abellotti]

Ping @rvsia, can you give this PR a try and see that it handles your use cases. Thank you. If all ok, the only thing left are the specs.

/cc @gtanzillo

[abellotti]

@gtanzillo re: the signature for count, I wonder if \<association>.count will cause problems later if some table indeed has a "count" attribute. Should we go with another syntax, maybe count.\<association> ?

[rvsia]

@abellotti Sources UI has to be able to sort Sources by count of their applications. And this case looks great with this PR! Thanks! And I like the proposed count.<association> syntax as it will support the count attribute. However, the same problem can occur if some entity has an association called count. (But the probability is probably too small.)

What about something like <association>:count:desc, <association>:count:asc? The : syntax could make clear distinction between entity attributes and computed attributes.

I have just one question to ask (and this is probably an issue for Sources API), but this wouldn't work when UI needs to sort Sources by source_type_id.product_name as there is no association between a source and its type, right?

[abellotti]

Hi @rvsia, thanks for confirming the use case.

I'm not crazy about using : for the attribute differentiation, would like to keep that for the asc/desc, the sort_by convention. I would introduce another character to differentiate, attribute vs. this computed one. maybe @

sort_by=association.attribute[:asc|desc]
sort_by=association.count[:asc|desc]    <=== the count attribute
sort_by=association.@count[:asc|desc]   <=== the computed association count.

/cc @gtanzillo thoughts ^

For the second, correct, it would not work. The association to the type needs to be exposed, including declaration in openapi spec to be queryable via GraphQL.

[miq-bot]

Checked commits https://github.com/abellotti/insights-api-common-rails/compare/af776b89e6ab50d629325f43fcbd9ca48077d79a~...e84051d4064c820f35daca06f6b8d252c23c5974 with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0 11 files checked, 0 offenses detected Everything looks fine. :star:

[abellotti]

Hi @bdunne, regarding: https://github.com/RedHatInsights/insights-api-common-rails/pull/141#pullrequestreview-354043759 I've been pondering on a resolve, trying to normalize the signatures of both:

• https://github.com/RedHatInsights/insights-api-common-rails/pull/141
• https://github.com/RedHatInsights/insights-api-common-rails/pull/159

In general, keeping the the current :asc|:desc suffix on the selected attribute is best. We can propose the additional layer for the association, and I have examples here below for the normalized signatures from those two PRs. The only one that I'm not sure of is when we do a sort_by on association attributes as well as direct. This would need to be prototyped/tested.

Showing here the Direct (today's implementation) and the w/Association (for the proposed signature).

Filtering:

    REST Direct:            filter[name][eq]=name1
    REST w/Association:     filter[source_type][name][eq]=name1

    GraphQL Direct:         filter: { name: { eq: "name1" } } 
    GraphQL w/Association:  filter: { source_type: { name: { eq: "name1" } } } 

    REST Direct:            filter[name][eq][]=name1&filter[name][eq][]=name2
    REST w/Association:     filter[source_type][name][eq][]=name1&filter[source_type][name][eq][]=name2

    GraphQL Direct:         filter: { name: { eq: ["name1", "name2"] } } 
    GraphQL w/Association:  filter: { source_type: { name: { eq: ["name1", "name2"] } } } 

Sorting:

    REST Direct:            sort_by=name
    REST w/Association:     sort_by[source_type]=name

    GraphQL Direct:         sort_by: "name"
    GraphQL w/Association:  sort_by: { source_type: "name" }

    REST Direct:            sort_by=name&sort_by=vendor:desc
    REST w/Association:     sort_by[source][]=name&sort_by[source][]=vendor:desc

    GraphQL Direct:         sort_by: [ "name", "vendor:desc" ]
    GraphQL w/Association:  sort_by: { source_type: ["name", "vendor:desc"] }

Current PR for Sorting against both direct and association attributes:

sort_by[]=application_types.display_name:asc&sort_by[]=id:desc

May be Problematic Signature (No idea if this will work):

      REST Direct w/Association:
          sort_by[application_types]=display_name:asc&sort_by[]=id:desc

      GraphQL Direct w/Association: sort_by:
          [ { application_types: "display_name:asc" }, "id:desc" ]

Let me know if this proposal seems reasonable/sane by you, and I'll give it a try.

[bdunne]

To completely align them I think we would need to bring the column name to the left side of the = and maybe only have the asc, desc or nil on the right side of the =.

[abellotti]

All doable -> https://github.com/RedHatInsights/insights-api-common-rails/pull/167 with association and attributes of the sortby in the hash coming in, GraphQL does not like much other than a-z,A-Z,0-9 and , so renaming @ count to just __count.

[abellotti]

Closing in preference for: #167