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

Being able to initialise a fully populated ROM::Struct #610

Open
mereghost opened this issue Sep 15, 2020 · 23 comments
Open

Being able to initialise a fully populated ROM::Struct #610

mereghost opened this issue Sep 15, 2020 · 23 comments
Assignees
Labels
Milestone

Comments

@mereghost
Copy link

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
Copy link
Member

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

@solnic
Copy link
Member

solnic commented Sep 17, 2020

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

@flash-gordon
Copy link
Member

@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
Copy link
Member

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

@solnic
Copy link
Member

solnic commented Sep 18, 2020

@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
Copy link
Member

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

@solnic
Copy link
Member

solnic commented Sep 18, 2020

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

@flash-gordon
Copy link
Member

@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
Copy link
Member

solnic commented Sep 22, 2020

@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
Copy link
Author

mereghost commented Oct 1, 2020

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

@solnic
Copy link
Member

solnic commented Oct 1, 2020

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

@mereghost
Copy link
Author

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
Copy link
Member

solnic commented Oct 6, 2020

@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 😆

@solnic solnic self-assigned this Jan 12, 2021
@cllns
Copy link

cllns commented May 12, 2021

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
Copy link
Member

solnic commented May 13, 2021

@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
Copy link

cllns commented May 14, 2021

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
Copy link
Member

solnic commented May 15, 2021

@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
Copy link
Member

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
Copy link

cllns commented May 16, 2021

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
Copy link
Member

solnic commented May 17, 2021

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

solnic commented May 18, 2021

@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.

@solnic solnic added this to the 6.0.0 milestone May 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Backlog
Development

No branches or pull requests

4 participants