AutoStruct does not respect type definition when creating

[hieuk09]

Describe the bug

This problem may be similar to #581. When using auto_struct, the object returned after creating a record doesn't use the configured type definition.

To Reproduce

Full script: https://gist.github.com/hieuk09/11feb1ef50061ef74ce5c5168e4fc0d9

Types::UpcaseString = Types::String.constructor { |input| input.upcase }

class Entities::User < ROM::Struct
  attribute :name, Types::UpcaseString
end

user = UserRepository.new.create(name: 'user1')
user.name
# => "user1"

user = UserRepository.new.users.map_to(Entities::User).first
user.name
# => "USER1"

Expected behavior

auto_struct should apply correctly upon creation.

Your environment

• Affects my production application: YES. Currently I'm avoiding this by monkey-patching ROM::StructCompiler to not overwriting my configuration.
• Ruby version: 2.7.2
• OS: MacOS

[flash-gordon]

Hm. This is actually intentional. Performance-wise it's not efficient to run constructor types every time you load an entity from storage. I would suggest you moving attribute transformations to the relation attribute :name, Types::String.meta(read: Types::String.constructor(&:upcase)) <- it's the "official way" of doing this.

WRT entity structs here's the list of my thoughts, semi-ordered:

1. Auto-generated structs must be instantiated via .allocate (.load in terms of dry-struct), it's a private API. For performance reasons. 99 of 100 times you don't need to run type checks and constructors. Obviously, it must be fully documented 😅
2. User-provided structs should rely on the same API, otherwise, it'd be weird: you have a performance penalty just for using custom structs.
3. Fall back to using .new when a struct with constructor types provided? I don't think it's a good idea, too magical.
4. Issue a warning when we detect there's a constructor type in the struct? It may make sense.

/cc @solnic

[hieuk09]

@flash-gordon: Actually, running constructor type when loading the entity is intentional here. In our code base, instead of the Type::UpcaseString, we use a Type::DecryptedString to decrypt PII in DB. Because ROM::Struct is immutable, it's not possible for us to lazily decrypt string and cache it; on the other hand, we don't want to decrypt the same string every time we read user.name either. Since we read PII almost every time we load user, decrypting string when constructing entity is a trade-off we made.

[flash-gordon]

@hieuk09 I get that, why wouldn't you use read types on relation attributes instead?

schema do
  attribute :name, Types::String.constructor { |v| encrypt(v) }.meta(read: Types::String.constructor { |v| decrypt(v) })
end

[hieuk09]

@flash-gordon: I tried your suggestion with the code above, and it doesn't seem to work:

module Entities
  class User < ROM::Struct
    attribute :name, Types::String.meta(read: Types::String.constructor(&:upcase))
  end
end

user_1 = repo.create(name: 'abc').name # => "abc"
user_2 = repo.users.map_to(Entities::User).first.name # => "abc"

I also tried the second suggestion (Types::String.constructor { |v| v.upcase }.meta(read: Types::String.constructor { |v| v.upcase })) but it doesn't work either. Did I miss anything here?

[flash-gordon]

@hieuk09 you need to put that code to the relation (users), not the struct. If you infer the schema, it'll be fine to combine inferred schemas with manually defined attributes

class Users < ROM::Relation[:sql]
  schema(infer: true) do
    attribute :name, ...
  end
end

See https://rom-rb.org/learn/core/5.2/schemas/#inferring-schemas

[hieuk09]

Thank you, this solution works for me. However, it seems weird for me that I need to define an attribute twice in different places for the same purpose. IMHO, this is error prone because if I forget to add the attribute configuration in either relation or entity, it'll cause error. Are there any ways to avoid the duplication?

[flash-gordon]

why though? The struct compiler uses schema attributes to build structs. You should be good with just

module Entities
  class User < ROM::Struct
  end
end

[hieuk09]

Hmm, currently we need to specify attribute in entity because we use fabrication gem, and it uses .new to initialize. We'll check out rom-factory to see if switching to it solves that problem.

Thank you very much @flash-gordon, I'll close this issue now.

[flash-gordon]

Yeah, it should help. Also, you can have simpler types in the entity compared to relations, attribute :name, Types::String will be enough.