Being able to initialise a fully populated ROM::Struct

[mereghost]

ROM::Structs have albeit having a schema (defined by query returns if I got it right) don't allow you to use them for initialisation.

# table authors (name:string, id:integer, created_at:datetime, updated_at:datetime)
class Author < ROM::Struct ; end
Author.new(name: 'Bob') # => <Author {}>

This makes it hard to test pieces of your code without touching the DB at all and therefore slowing the testing down. Is it possible to offer some similar functionality (maybe as part of rom-sql :/ )?

Author.new(name: 'Bob') # => <Author {name: 'Bob'}>

[flash-gordon]

@mereghost rom-factory does that, have you seen it?

[solnic]

@flash-gordon the idea is to be able to do it via class constant. Thoughts?

[flash-gordon]

@solnic type metadata will be missing, it can be a problem in some cases. For instance, you won't be able to notice column renaming. Otherwise, it's possible with method_missing and overriding of #to_s

[flash-gordon]

though I think I would go with a different name, Struct.build or something

[solnic]

@solnic type metadata will be missing, it can be a problem in some cases. For instance, you won't be able to notice column renaming

@flash-gordon not sure if I follow - we'd use actual struct classes generated by the struct compiler, they have meta-data. I'd imagine that if you do User.build(name: "Jane") then the factory would search for the canonical representation of that struct class (basically going to users relation and grabbing its users.mapper.model) and using that. So, essentially a little bit of magic behind the scenes.

I like Struct.build btw.

[flash-gordon]

@solnic linking User to a container would require some sort of a global state, this concerns me somewhat :)

[solnic]

@flash-gordon but it's already "linked" through struct_namespace and class name inference, isn't it?

[flash-gordon]

@solnic no, it's the other way around, container references classes. Currently, User is an empty class that can be easily shared by multiple containers with (possibly) different schemas

[solnic]

@flash-gordon you're right. I guess we should stick to factories after all. @mereghost would you be OK with that? Please remember that User is only a placeholder class, the actual struct classes used at runtime are subclasses of that User class and they are created on-the-fly, based on relation schemas.

[mereghost]

I took a look at rom-factory but I'm under the impression that it ends up writing to the DB, right?

[solnic]

@mereghost no, you can ask for an in-memory struct via ie Factory#structs[:user]

[mereghost]

I'm looking at it using my hanami/model's glasses:

If someone is using their own structs (struct_namespace) there (even with a couple of methods inside), they will probably be caught off-guard by not being able to initialize them.

What model used to do was to look at the relations (and associations) and craft a mapper for that relation and a schema for the entity, registered as :entity, that we would use by default. For that we did a couple of workarounds that looked like 9and were) an ugly hack but that created a nice developer experience.

That being said: this is OK as it seems to be the preferred rom way to do it and if we want the best for both projects we both need to let go of previous assumptions. =)

But I'd love to see this piece of dev UX to improve, either as plugin or as part of rom itself. =)

[solnic]

@mereghost every rom-rb user must learn how auto-structs work though, so pretending that an abstract entity class is something that it's not would be confusing. We can make it nice and easy though, but unfortunately we shouldn't rely on abstract class constructors for that because that would give you the wrong impression and cause confusion. I'm totally for adding support for this to rom core btw. I cringe every time I do relation.mapper.model.new 😆

[cllns]

If someone is using their own structs (struct_namespace) there (even with a couple of methods inside), they will probably be caught off-guard by not being able to initialize them.

This just got me, so I agree. I expected ROM::Struct to behave like Dry::Struct (and every other normal Ruby class).

If creating a Struct with .new on the class isn't possible, could we possibly raise an Exception and provide an indication to users suggesting to use rom-factory (by catching it in internal library code) and possibly linking to a doc that explains why? I expect most Ruby users will reach for .new since it's a basic and ubiquitous part of Ruby.

[solnic]

@cllns this is one of my top priorities in rom 6.0. Struct implementation in rom is a pretty advanced piece TBH. I understand people's expectations because it shares attribute foundation with Dry::Struct but these are not normal classes. Just like ActiveRecord models are not normal classes. It's something a rom-rb user has to learn. We can improve the situation by adding meaningful exceptions like you suggested though, so that's what I'll most likely do.

[cllns]

Great to hear! Maybe it’s more appropriate to think of ROM::Struct as a “Struct Definition” class rather than a Struct class itself (like how Dry::Struct, ::Struct, ::OpenStruct, etc. use the word).

Maybe renaming it something like ROM::StructDefinition, ROM::MetaStruct, ROM::AutoStruct could help signal to users that they’re a new thing to learn. Just an idea though, I don’t understand the domain & usage well enough to know if that’s a good idea.

[solnic]

@cllns this is actually a really good idea. To give you a quick explanation - the struct class you define by inheriting from ROM::Struct is never instantiated. ROM creates sub-classes of your class for every unique schema. I'm not sure how to rename it to make it clear that this is what's happening though. Hard to come up with a short word 🙂

It just occurred to me though that maybe it would be possible to have your classes use the default schema and already be concrete. Then we could generate an abstract class on-the-fly for any non-default schema and derive dedicated classes from this abstract class. @flash-gordon thoughts on this idea? Have you considered this maybe in the past?

[flash-gordon]

ROM::Abstruct :)

It just occurred to me though that maybe it would be possible to have your classes use the default schema and already be concrete. Then we could generate an abstract class on-the-fly for any non-default schema and derive dedicated classes from this abstract class. @flash-gordon thoughts on this idea? Have you considered this maybe in the past?

But what would you do with accessors generated from the base class? And then, what if two containers with different schemas use the same base struct? Which one would be considered the default one? Currently, we have clear semantics, as I can see, we'll have to trade it for better ergonomics. Another argument is structs are not tied to schemas hence, they can be loaded separately. What if you instantiate a struct without loading a schema somewhere else.

It's possible to come up with a safe and predictable API I think, it would require relying on effects (not necessary dry-effects, thread local variables would work just as well) like for most tricky things

ROM::Struct.with_container do # you call it somewhere at the top, in middleware, in RSpec.around(:all), etc
  Entities::User.new() # relies on implicitly passed container
end

That much I can suggest in terms of well-defined API :)

[cllns]

I think most Structs in most codebases (my impression) will have just one associated schema. Can we privilege that use-case, but still retain the power & semantic correctness of supporting many schemas?

I’m working at the edge of my understanding, so forgive me if this doesn’t make sense, but this has been productive so far and I think we’re making progress on this (as @flash-gordon noted) semantics-ergonomics balance.

Could we use container/schema names somehow, with a registry? (e.g. accessed as Entities::User[:default] or just Entities::User for :default).

Then later, when another container/schema is added, they must be named (e.g. accessed as Entities::User[:redis]). Seems like this might use a little bit of global state, but in a sensible way.

[solnic]

I would prefer not to sacrifice ergonomics because of less common use cases, assuming we can support both. I think structs should be per container anyway. If you want shared classes then it should be up to you to set it up. My idea in theory sounds simple:

# with this relation
class Users < ROM::Relation[:sql]
  schema(:users) do
    attribute :id, Types::PrimaryKey
    attribute :name, Types::String
  end
end

# having this struct
class User < ROM::Struct
end

# during setup, rom would define attributes for this default struct assuming
# Users's default schema becomes this struct's schema, so
User.schema # matches Users.schema

# then, if you do something like
users.select(:name)

# this would result in
abstract_class = User.superclass
# define attributes from the dynamic schema on-the-fly
abstract_class.schema # this now matches users.select(:name).schema

# Everything is of course cached at run-time

So, in which scenario this would be problematic? Please remember that I want to make Structs first class citizens in ROM 6.0 setup process, so auto-register will deal with them etc.

I also like @cllns's idea that we can leverage the already established pattern with Class#[] and have the ability to provide container's identifier.

[flash-gordon]

Got an idea, here's a sketch

module AbstractEntities
  class User < ROM::Struct
  end
end

# somewhere else
App.boot(:persistence) do |container|
  init do
    rom = ...
    Entities = rom.make_structs(AbstractEntities)
  end
end

# then
Entities::User.new(name: 'John Doe')

This way we "offload" global state management to users but in a controlled way. WDYT?

[flash-gordon]

My idea in theory sounds simple:

I didn't get it :( How is

# having this struct
class User < ROM::Struct
end

compatible with

# this would result in
abstract_class = User.superclass
# define attributes from the dynamic schema on-the-fly
abstract_class.schema # this now matches users.select(:name).schema

?
User.superclass is ROM::Struct. Besides,

user = users.select(:name).by_name('John').limit(1).one
user.is_a?(User) # false!

[solnic]

@flash-gordon sorry that was a confusing mental shortcut. I meant that we need an abstract class that is used for building dedicated structs BUT have the default "User" as a concrete implementation already. This means that we'd generate something like AbstractUser < ROM::Struct and have User < AbstractUser. So it's what you showed more or less in your example.

[cllns]

I just came across this issue again, and I think it'd help a lot of we include the object_id of the class in the inspect output. So then people would see something like

article1.class # => ROM::Struct::Article<11020>
article2.class # => ROM::Struct::Article<80600>

Instead, now they show the same string even though they're different objects. Usually in Ruby we expect classes with the same name are the same object.

Of course, they'd change every time but I think that's fine for inspect output.

[solnic]

This is something we could address in 6.0. I'd be for introduction of the Abstract prefix and then User < AbstractUser. I am not sure yet what to do with classes that are generated for non-canonical schemas though. Your suggestion to add object_id sounds like a simple solution so maybe that's the way to go :)

[flash-gordon]

There's also a new feature in ruby for classes having non-standard naming, it may be useful here: https://github.com/ruby/ruby/pull/7483/files#diff-71f34ef53b0d05e06f87bf7a7f89a88ad136325a33864985ee48895ebc90394eR9

[cllns]

Related to this issue: is there a way to use a verifying double with RSpec (instance_double) and ROM 5 structs that infer their schema from the relation?

article = instance_double(MyApp::Structs::Article, id: 1)

Shows this error

#   Failure/Error:    the MyApp::Structs::Article class does not implement the instance method: id

If I add attribute :id, Types::Integer to the Struct's schema then it works though.

[solnic]

@cllns you need to use the struct class that is actually used by the mapper, ie instance_double(articles.mapper.model, id: 1) should work