Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Problem with a polymorphic table query #115

Closed
mrship opened this issue Dec 14, 2016 · 13 comments
Closed

Problem with a polymorphic table query #115

mrship opened this issue Dec 14, 2016 · 13 comments

Comments

@mrship
Copy link

mrship commented Dec 14, 2016

We have a table (snapshots) that is reused across several entities. The relationship to the other entities is specified by a combination of parent_id and parent_type in that table.

I have set up two relations (below) so that I can get as Snapshot for a Question.

This is all working well, if I use the method directly on the Snapshot relation to get the snapshot. However, if I use that method using combine_children in a QuestionRepo then it does not correctly setup the snapshot attribute in the result set that ROM returns.

Here's the code:

class Questions < ROM::Relation[:sql]
  dataset :questions
end

class Snapshots < ROM::Relation[:sql]
  dataset :snapshots

  def for_question(questions)
    where(parent_id: questions.map { |q| q[:id] }, parent_type: "question")
  end
end

[1] db = ROM.env.gateways[:default].connection
=> #<Sequel::SQLite::Database: "sqlite::memory/store">
[2] db[:snapshots].to_a
=> [{:id=>1,
  :parent_id=>1,
  :parent_type=>"question",
  :figure_id=>1,
  :figure_type=>"charts",
  :illustration=>"{}",
  :created_at=>2016-12-14 10:55:00 +0000,
  :updated_at=>2016-12-14 10:55:00 +0000}]
[3] snapshots.for_question(questions).one
=> #<ROM::Struct[Snapshot] id=1 parent_id=1 parent_type="question" figure_id=1 figure_type="charts" illustration="{}" created_at=2016-12-14 10:55:00 +0000 updated_at=2016-12-14 10:55:00 +0000>
[4] questions.combine_children(one: snapshots.for_question).to_a
=> [#<ROM::Struct[Question] id=1 topic_id=nil title="How are my employees performing?" short_title="Employee performance" clarification="Rejected opportunities are excluded from the achievement figures." methodology=nil sage_klass="Lighthouse::Sages::Questions::Simpleton" order=nil created_at=2016-12-14 10:54:58 +0000 updated_at=2016-12-14 10:54:58 +0000 snapshot=nil>]

As you can (hopefully) see, I have the correct data in the database [#1] and [#2], the method on the Snapshots relation works correctly [#3], but the combine_children call does not.

Any thoughts? Am I missing some setup step? Or is this not going to work as I expect?
@solnic
Copy link
Member

solnic commented Dec 14, 2016 •
edited
Loading

It doesn't work because inferred combine key is question_id and it's missing in the resulting tuples. You can either specify the key manually using #combine or you can still use combine_children but you need to rename parent_id to question_id in for_questions so something like that:

def for_questions(questions)
  rename(parent_id: :question_id).where(parent_id: questions.pluck(:id), parent_type: "question")
end

Lemme know if that works. I should mention that in rom-repository 1.0.0 this will raise an error telling you that the combine key is missing and how you can make it work :)

@mrship
Copy link
Author

mrship commented Dec 14, 2016

That makes sense, and I had something similar at one stage where I did a select_append question_id but, with your changes, I then get:

ArgumentError: unknown keyword: question_id
from /Users/shipmana/.rbenv/versions/2.2.5/lib/ruby/gems/2.2.0/gems/rom-repository-0.3.1/lib/rom/repository/struct_attributes.rb:39:in `initialize'

Implying that I can't create the ROM::Struct with the unknown key of question_id.

I'll dig deeper.

@solnic
Copy link
Member

solnic commented Dec 14, 2016

@mrship ah right, this should be a view with a header:

view(:for_questions, %i[id question_id ... ]) do |questions|
  rename(parent_id: :question_id).where(parent_id: questions.pluck(:id), parent_type: "question")
end

@solnic
Copy link
Member

solnic commented Dec 14, 2016

btw rom-sql deliberately doesn't support polymorphic associations because we do not want to promote features that throw data integrity out the window; however, since folks may have legacy databases, we can easily provide a plugin that you can enable that will setup associations for polymorphic relations by following how it's done in ActiveRecord.

@mrship
Copy link
Author

mrship commented Dec 14, 2016

OK, that helps in that I now do get a snapshot back (see below), however, I then try to map that result set as an entity, using .as(Question) (where Question is an entity class) then it fails with:

#<TypeError: #<ROM::Struct[Snapshot] id=1 question_id=1 parent_type="question" figure_id=1 figure_type="charts" illustration="{}" created_at=2016-12-14 15:24:28 +0000 updated_at=2016-12-14 15:24:28 +0000> is not a symbol nor a string>

in rom/relation/loaded.rb

Any thoughts on why a mapping wouldn't work with that struct?

The successful bit :)

questions.combine_children(one: snapshots.for_question).to_a
=> [#<ROM::Struct[Question] id=1 topic_id=nil title="How are my employees performing?" short_title="Employee performance" clarification="Rejected opportunities are excluded from the achievement figures." methodology=nil sage_klass="Lighthouse::Sages::Questions::Simpleton" order=nil created_at=2016-12-14 12:11:15 +0000 updated_at=2016-12-14 12:11:15 +0000 snapshot=#<ROM::Struct[Snapshot] id=1 question_id=1 parent_type="question" figure_id=1 figure_type="charts" illustration="{}" created_at=2016-12-14 12:11:18 +0000 updated_at=2016-12-14 12:11:18 +0000>>]```

@solnic
Copy link
Member

solnic commented Dec 15, 2016

@mrship what's your entity class?

@mrship
Copy link
Author

mrship commented Dec 15, 2016

Sorry, I appreciate that it wasn't a particularly good error report re: the mapping; I'll try and isolate it further and provide some more detail.

@mrship
Copy link
Author

mrship commented Dec 17, 2016 •
edited
Loading

My entity class is a Dry::Struct, along the lines of:

class Question < Dry::Struct
  constructor_type :schema
  attribute :snapshot, Types::Snapshot.optional
  attribute :charts, Types::Strict::Array.member(Chart).default { [] }
end

I have a custom type to create the correct snapshot object. I follow a similar pattern for charts (see above) that I pull back in my object graph with a combine_children/many, e.g.

questions
  .combine_children(
    many: {
      charts: charts,
    },
    one: {
      snapshot: snapshots.for_questions,
    }
  )

This works really well for the (many) charts, but not for one snapshot. Digging deeper it seems I'm being bitten by activesupport monkeypatching Object#try (which is included via another gem I'm using). The (snipped) backtrace for the error shows that I end up in ActiveSupport Object#try when calling try from dry-types. Interestingly I don't follow the same dry-struct code path for charts as it seems an Types::Strict::Array.member doesn't run the try method in the same way.

To be honest, I'm deep in the weeds here, so I'm not sure how to deal with this. I can make my snapshot an array (with one element) and it works fine (following the same pattern as for charts) but I'd prefer to keep it as a single relationship if possible.

Any thoughts? (Other than killing activesupport with extreme prejudice, which unfortunately isn't an option)

 "/Users/shipmana/.rbenv/versions/2.2.5/lib/ruby/gems/2.2.0/gems/activesupport-4.2.7.1/lib/active_support/core_ext/object/try.rb:64:in `try'",
 "/Users/shipmana/.rbenv/versions/2.2.5/lib/ruby/gems/2.2.0/gems/dry-types-0.9.3/lib/dry/types/sum.rb:76:in `block in try'",
 "/Users/shipmana/.rbenv/versions/2.2.5/lib/ruby/gems/2.2.0/gems/dry-types-0.9.3/lib/dry/types/constrained.rb:44:in `try'",
 "/Users/shipmana/.rbenv/versions/2.2.5/lib/ruby/gems/2.2.0/gems/dry-types-0.9.3/lib/dry/types/sum.rb:75:in `try'",
 "/Users/shipmana/.rbenv/versions/2.2.5/lib/ruby/gems/2.2.0/gems/dry-types-0.9.3/lib/dry/types/sum.rb:70:in `call'",
 "/Users/shipmana/.rbenv/versions/2.2.5/lib/ruby/gems/2.2.0/gems/dry-types-0.9.3/lib/dry/types/hash/schema.rb:75:in `block in coerce'",
 "/Users/shipmana/.rbenv/versions/2.2.5/lib/ruby/gems/2.2.0/gems/dry-types-0.9.3/lib/dry/types/hash/schema.rb:88:in `block in resolve'",
 "/Users/shipmana/.rbenv/versions/2.2.5/lib/ruby/gems/2.2.0/gems/dry-types-0.9.3/lib/dry/types/hash/schema.rb:86:in `each'",
 "/Users/shipmana/.rbenv/versions/2.2.5/lib/ruby/gems/2.2.0/gems/dry-types-0.9.3/lib/dry/types/hash/schema.rb:86:in `resolve'",
 "/Users/shipmana/.rbenv/versions/2.2.5/lib/ruby/gems/2.2.0/gems/dry-types-0.9.3/lib/dry/types/hash/schema.rb:73:in `coerce'",
 "/Users/shipmana/.rbenv/versions/2.2.5/lib/ruby/gems/2.2.0/gems/dry-types-0.9.3/lib/dry/types/hash/schema.rb:27:in `call'",
 "/Users/shipmana/.rbenv/versions/2.2.5/lib/ruby/gems/2.2.0/gems/dry-types-0.9.3/lib/dry/types/constructor.rb:39:in `call'",
 "/Users/shipmana/.rbenv/versions/2.2.5/lib/ruby/gems/2.2.0/gems/dry-struct-0.1.1/lib/dry/struct/class_interface.rb:77:in `new'",

@solnic
Copy link
Member

solnic commented Dec 17, 2016 via email

@mrship
Copy link
Author

mrship commented Dec 17, 2016

Aha! your comment about optional allowed me to progress. I can specify this as:

attribute :snapshot, Types::Snapshot.default { nil }

to get around the issue I have with ActiveSupport.

So, thanks for all the help. Let me also say that ROM is awesome. I'm enjoying the freedom to do DDD that it gives me.

It could definitely do with better documentation though :)

@mrship mrship closed this as completed Dec 17, 2016
@solnic
Copy link
Member

solnic commented Dec 17, 2016 via email

@mrship
Copy link
Author

mrship commented Dec 22, 2016

Sorry for the delay in replying, but yes that's exactly the issue I was having when declaring an attribute as .optional I was ending up in ActiveSupport's try method and that was throwing an error about the ROM::Struct not being a symbol or a string.

@solnic solnic mentioned this issue Dec 23, 2016
Closed
@solnic
Copy link
Member

solnic commented Dec 23, 2016

@mrship OK I suspect it's a bug in structs + optional. I reported an issue in dry-struct dry-rb/dry-struct#26

@blelump blelump mentioned this issue Mar 6, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@solnic @mrship