search

rom-rb / rom

Data mapping and persistence toolkit for Ruby
https://rom-rb.org
MIT License
2.08k stars 161 forks source link

Struct compiler now checks if a class exists before creating a new one #496

Closed radar closed 5 years ago

radar commented 6 years ago

This is related to https://github.com/dry-rb/dry-core/pull/26. I am still seeing an issue in some projects, which you can still reproduce using the steps in dry-rb/dry-core#25.

A class may already exist and just not be available (yet) because it hasn't been autoloaded. The StructCompiler class should call const_get in an attempt to load this class. If it can load the class, then it should use this class and not go to Dry::Core::ClassBuilder to build an entirely new class.

In my little script to reproduce this issue:

repo = ProjectRepository.new(ROM.env)
project = repo.projects.to_a.first
puts project.class.to_s == "Projects::Project"
puts project.respond_to?(:to_model)

This shows up "false" and "true". I believe this is because the Projects::Project constant is not being autoloaded in Dry::Core::ClassBuilder due to the line I pointed out in dry-rb/dry-core#25. I am on Ruby 2.5, and so that line isn't called.

Reviewing my decision there, I think making StructCompiler attempt to find the class is a better course of action, as ClassBuilder would then only be used in the cases where we had to actually build a class. This is the route I've gone down with this PR.

With this patch applied, my little script shows up "true" and "true", indicating that the behaviour is correct... at least according to that script.

I couldn't work out a non-ghetto way of clearing the constant in the tests. Sorry.

radar commented 6 years ago

@solnic Am I on the right track to solving this problem here? I just ran into it again today.

flash-gordon commented 6 years ago

@radar no, it's wrong, I wonder why our tests are even passing 😟 The idea is to use user-provided structs as base classes, whereas your changes modify the base class itself. In a real-world application rom creates more than one subclass and defines required attributes in every one of them. With your changes, it will try to define them all in a single struct class, this will end up with an error.

We need to improve our test suite.

david-pm commented 5 years ago

Was seeing errors:

[9] pry(main)> project.persisted?
ROM::Struct::MissingAttribute: undefined method `persisted?' for #<Projects::Project id=1 name="test project"> (attribute not loaded?)
from /.rvm/gems/ruby-2.5.1/gems/rom-mapper-1.2.1/lib/rom/struct.rb:112:in `rescue in method_missing'

Adding this branch to my Gemfile fixed the issue but seems like it will not be the longterm solution 😢 gem 'rom-mapper', github: 'radar/rom', branch: 'build-once'

solnic commented 5 years ago

I'm going to close this as it's not the right solution. Please report an issue describing the problem and we can think about addressing it properly. My gut feeling is telling me this should be solved in rom-rails because it's related to const auto-loading and code reloading.

nfstern commented 4 years ago

@radar: Working thru your ExplodingRails and you said to comment here on whether the fix worked. adding gem 'rom-mapper', github: 'radar/rom', branch: 'build-once' does not seem to have fixed the problem.