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.

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

Select columns are always overidden when using combine #287

Open
blelump opened this issue Apr 18, 2018 · 5 comments
Open

Select columns are always overidden when using combine #287

blelump opened this issue Apr 18, 2018 · 5 comments

Comments

@blelump
Copy link

blelump commented Apr 18, 2018

Hello,

when combining an aggregate, ie:

class UserRepo < ROM::Repository[:users]
  struct_namespace ::Entities
  def by_id(id)
    users.combine(employees_users: :employee).to_a
  end
end

and having employees relation:

  conf.relation(:employees) do
    schema(:employees, infer: true) do
      associations do
      end
    end

    dataset do
      select_append{ Sequel[:name].as(:a) }
    end
  end

the selected columns within dataset block are always ignored and the defaults are used, ie:

SELECT `employees`.`id`, `employees`.`name` FROM `employees` WHERE (`employees`.`id` IN (5))

However, when you query using the relation, ie:

rom.relations[:employees].last
=> SELECT *, `name` AS 'a' FROM `employees` LIMIT 1

it works as expected.

Complete example:

module Entities
end

rom = ROM.container(:sql, 'sqlite::memory') do |conf|
  conf.gateways[:default].use_logger(Logger.new($stdout))


  conf.default.create_table(:users) do
    primary_key :id
    column :login, String
  end

  conf.default.create_table(:employees) do
    primary_key :id
    column :name, String, null: false
  end

  conf.default.create_table(:employees_users) do
    foreign_key :verifable_id, :employees
    foreign_key :user_id, :users
  end

  conf.relation(:users) do
    schema(:users, infer: true) do
      associations do
        has_many :employees_users
        has_many :users, through: :employees_users
      end
    end
  end

  conf.relation(:employees) do
    schema(:employees, infer: true) do
      associations do
      end
    end

    dataset do
      select_append{ Sequel[:name].as(:a) }
    end
  end

  conf.relation(:employees_users) do
    schema(:employees_users, infer: true) do
      associations do
        belongs_to :user, foreign_key: :user_id
        belongs_to :employee, foreign_key: :verifable_id
      end
    end
  end
end

class UserRepo < ROM::Repository[:users]
  struct_namespace ::Entities
  def by_id(id)
    users.combine(employees_users: :employee).to_a
  end
end

employee = rom.relations[:employees].changeset(:create, name: 'Jane', id: 5).commit
rom.relations[:employees].changeset(:create, name: 'Jane', id: 8).commit

user = rom.relations[:users]
  .changeset(:create, login: '[email protected]')
  .commit

rom.relations[:employees_users]
  .changeset(:create, user_id: user[:id], verifable_id: employee[:id]).commit

repo = UserRepo.new(rom)

user = repo.by_id(1)
@abrthel
Copy link
Contributor

abrthel commented May 2, 2018

I literally ran into this exact same problem a few moments ago.

An additional gist showing this bug in action with rom v4.2.1 & rom-sql v2.4.0:
Combine overrides default dataset

@abrthel
Copy link
Contributor

abrthel commented May 3, 2018

After a bit of sleuthing, I believe the issue lies with rom schemas, If you change the selections of the relation at the dataset level then the relations schema doesn't get updated. Then when the schema gets updated with parent id during the combine the new selections override the selections from the default dataset.

@solnic
Copy link
Member

solnic commented May 4, 2018

Does it work when you define the schema explicitly? I think this is the only way to make it work now. Schemas are inferred from canonical db schemas, and if you override the dataset, it's gonna be impossible to infer the schema reliably from that dataset, as it only provides column names IIRC. Unless I'm mistaken.../cc @flash-gordon

@abrthel
Copy link
Contributor

abrthel commented May 4, 2018

In my case explicit schema definitions didn't work and as far as I know, there isn't a way to explicitly define attributes on a schema that targets another table. The problem is that when rom-sql qualifies the column name it points to the wrong table.

eg:
SELECT "regions"."slot_cap" FROM ...
vs what should be
SELECT "regions_start_pos"."slot_cap" FROM ...

My only solution so far was to either "fix" the dataset every time after modifying it or using hackity hack hacks to replace the relation with a fixed up schema in the container.

... it's gonna be impossible to infer the schema reliably from that dataset

Agreed, I don't think that's possible and I'm sure the solution will require some type of manual intervention which only makes sense for usecases like this.

@abrthel
Copy link
Contributor

abrthel commented Jun 22, 2018

Thanks to @mrship I've stumbled upon a solution to this issue.

TL;DR; I've updated my reproduction script to show the fix
Combine overrides default dataset FIXED

The Problem

During the finalization process the default dataset for the relation can be manipulated and columns/attributes from other tables can be joined into the relation. When using the resulting relation to query for its data the results are returned as you'd expect - as in all extra added attributes are returned. However if you then combine that relation into another relation suddenly all of your extra attributes are no longer apart of the returned results.

Whats Happening

Modifying the dataset during finalization does not also modify the schema. The reason we don't see the issue right after finalization is because the schema isn't called to modify the relation probably because it's just assumed they're in the same state.

Then when combining the relation into another relation the schema is applied to the relation with the modified dataset and the extra joined attributes are discarded as there are no definitions for those attributes in the schema.

A Solution

When joining two tables at the dataset level, the joined attributes need to be explicitly defined in the relations schema. However to do that, each of those attributes will also need to be explicitly qualified to the joined table's name through the use of the :qualified option. EG:

class Region < ROM::Relation[:sql]
  schema(infer: true) do
    attribute :base_gdp, Types::Strict::String.meta(qualified: :regions_start_pos)
  end
  dataset do
   select(:id, :name, :base_gdp).join(:regions_start_pos, region_id: :id)
  end
end

As you can see other attributes for Region are inferred like normal but the base_gdp is explicitly defined and qualified to a table that has a dataset but doesn't have a corresponding relation in the rom container.

Solnic did mention this idea above but until mrship came along I didn't know how to qualify an attribute to another table during finalization.

Final Notes

  1. I recommend adjusting the general docs at rom-rb.org to cover this use case as it has come up multiple times now.

  2. I haven't really thought this one through so it might not work but during finalization, rom could compare the results of dataset.columns to the attributes available in the schema. When mismatched throw an error. Possible edge cases would be column renames.

  3. I also think we need to think about and develop a story/api around easily referencing attributes from relations that don't exist in the rom container but do at the dataset/database level. Sometimes devs just don't want to create a relation for a certain table but need to access that table all the same. In my case, I don't have control over the database so I can't hide the nastiness of some of my tables behind a view. On the flip side I know I don't want to ever reference a particular table unless it's joined to a table that I do have a relation in rom for. The solution above is a good compromise but working with attributes outside of available rom relations is still a real pain. eg
    .select_append(adjustments[:id].qualified(:manager_adjustments).as(:manager_adjustments_id))
    is needed just to append an attribute into the schema that references another table and column name.

  4. This may already be possible but rom could explicitly support qualifying attributes when manually defining an attribute instead of manipulating they type meta. I think it can be confusing to new comers and most meta manipulations that I can think of have an explicit dsl api.

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

No branches or pull requests

3 participants