Skip to content

Add STI relationship option#1359

Open
lgebhardt wants to merge 2 commits into
release-0-10from
sti_v0-10
Open

Add STI relationship option#1359
lgebhardt wants to merge 2 commits into
release-0-10from
sti_v0-10

Conversation

@lgebhardt

Copy link
Copy Markdown
Contributor

This fixes a few issues I found with STI

All Submissions:

  • I've checked to ensure there aren't other open Pull Requests for the same update/change.
  • I've submitted a ticket for my issue if one did not already exist.
  • My submission passes all tests. (Please run the full test suite locally to cut down on noise from travis failures.)
  • I've used Github auto-closing keywords in the commit message or the description.
  • I've added/updated tests for this change.

New Feature Submissions:

  • I've submitted an issue that describes this feature, and received the go ahead from the maintainers.
  • My submission includes new tests.
  • My submission maintains compliance with JSON:API.

Bug fixes and Changes to Core Features:

  • I've included an explanation of what the changes do and why I'd like you to include them.
  • I've provided test(s) that fails without the change.

Test Plan:

Reviewer Checklist:

  • Maintains compliance with JSON:API
  • Adequate test coverage exists to prevent regressions
@lgebhardt lgebhardt marked this pull request as ready for review March 23, 2021 15:10
@lgebhardt lgebhardt requested a review from dgeb March 23, 2021 15:11

@bf4 bf4 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is what @JamesGlover was referring to

pluck_fields << Arel.sql("#{concat_table_field(linkage_table_alias, primary_key)} AS \"#{linkage_table_alias}_#{primary_key}\"")

if linkage_relationship.sti?
pluck_fields << Arel.sql("#{concat_table_field(linkage_table_alias, 'type')} AS \"#{linkage_table_alias}_type\"")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pluck_fields << Arel.sql("#{concat_table_field(linkage_table_alias, 'type')} AS \"#{linkage_table_alias}_type\"")
pluck_fields << Arel.sql("#{concat_table_field(linkage_table_alias, klass.inheritance_column)} AS \"#{linkage_table_alias}_type\"")

right?

@JamesGlover JamesGlover Jun 9, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think at this point klass is the resource, not the ActiveRecord::Base class, so you'd also need to define an inheritance_column method on JSONAPI::BasicResource.

Although in practice you'd probably want to use _inheritance_column instead, to be consistent with _primary_key.

# lib/jsonapi/basic_resource.rb
def inheritance_column(key)
  @_inheritance_column = key.to_sym
end

def _inheritance_column
  @_inheritance_column ||= _default_inheritance_column
end

def _default_inheritance_column
  @_default_inheritance_column ||=_model_class.respond_to?(:inheritance_column) ? _model_class.inheritance_column : :type
end
]

if relationship.sti?
pluck_fields << Arel.sql("#{concat_table_field(resource_table_alias, 'type')} AS #{resource_table_alias}_type")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pluck_fields << Arel.sql("#{concat_table_field(resource_table_alias, 'type')} AS #{resource_table_alias}_type")
pluck_fields << Arel.sql("#{concat_table_field(resource_table_alias, klass.inheritance_column)} AS #{resource_table_alias}_type")

right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants