Decouple attribute alias from type definition #512

waiting-for-dev · 2018-12-20T10:39:03Z

This PR is created because of this thread in the forums:

https://discourse.rom-rb.org/t/ability-to-alias-an-attribute-but-keeping-type-inferred/289

Basically, it decouples an attribute alias from its type, so it will be
possible to define an alias but still make use of schema inference.

This is still a WIP, but I'm looking for some feedback in order not to do extra work.

Some considerations I'd like to have your thoughts:

The DSL is still not adapted. I haven't tested it, but I guess it won't work
defining an attribute without specifying its type, because the main purpose
of this PR is to be able to do a simple:

attribute :foo, alias: :bar
The option to define an alias is called alias:. This is because as: can't
be freely used because it is already a method name and dry-initializer
creates methods from option names. We could use method renaming feature in
dry-initializer for that, but then we have to juggle with the renaming in
the rest of code, like in aliased and meta methods, and it is a bit ugly.
The handicap is that in other places of the API as: is used for similar
purposes, like in association aliasing.
alias method has been removed because it is defined out of the box by
dry-initializer.
The attribute ast will now always return alias: nil when an alias is not
defined. This is different from before, where alias key was not present in
those situations. In my opinion, it makes more sense to always return all the keys.
The test for returning aliased attribute identified by canonincal name has
been removed, because I think that, being now an option and not part of the
type, it makes no sense.
I have redefined ROM::SQL::Function#name method in the repository
integration tests. This should be changed in rom-sql project. How would yo
go with it?
I guess that it would also make sense to move wrapped from the meta to an
option. Would you like to change it also in this PR?
What do you think about primary_key meta? Should we consider it part of the
type (meta) or also an option? Even if it related with the value (the value
being unique), it is so only considering the rest of values. It is like
dynamic typing. Anyway, it would be more convenient to have it as option, I
guess.

Thanks!

solnic · 2018-12-20T12:46:53Z

The test for returning aliased attribute identified by canonincal name has
been removed, because I think that, being now an option and not part of the
type, it makes no sense.

Could you elaborate a bit more here?

waiting-for-dev · 2018-12-20T15:36:33Z

The test for returning aliased attribute identified by canonincal name has
been removed, because I think that, being now an option and not part of the
type, it makes no sense.

Could you elaborate a bit more here?

Well, I guess this test existed in order to be sure than aliasing an attribute didn't change the name in the meta of the type. But now they are two different concepts. One thing is the type of an attribute and another thing are their options (like alias:). So, I think it makes no sense to test that the change in the type doesn't affect the options... why should it? I think that keeping the test just reflects the development history (the fact that the alias was part of the type before). Furthermore, rename seems very well tested in rename_spec file.

But maybe I'm missing something 🙂

solnic · 2018-12-30T13:08:39Z

Well, I guess this test existed in order to be sure than aliasing an attribute didn't change the name in the meta of the type.

This test exists because it checks we can still access attributes using canonical names, and rename_spec doesn't cover this scenario, so it seems like we should not remove the test.

waiting-for-dev · 2018-12-30T21:07:00Z

Makes sense. I readded the test, fixed rubocop issues and forced the push rebasing from master.

There is still more to review, however. Besides the points I mentioned in the first comment, we should also decide about the #inspect output for Attribute now that it has more options.

solnic · 2018-12-31T12:44:12Z

Besides the points I mentioned in the first comment, we should also decide about the #inspect output for Attribute now that it has more options.

We could do:

def inspect
  %(#<#{self.class}[#{type.name}] #{meta.merge(options).map { |k, v| "#{k}=#{v.inspect}" }.join(' ')}>)
end

core/lib/rom/attribute.rb

waiting-for-dev · 2018-12-31T16:00:43Z

I added a commit for each change. We can squash them later if needed.

Besides the points I mentioned in the first comment, we should also decide about the #inspect output for Attribute now that it has more options.

We could do:
def inspect
  %(#<#{self.class}[#{type.name}] #{meta.merge(options).map { |k, v| "#{k}=#{v.inspect}" }.join(' ')}>)
end

As with attribute ast, inspect will always include alias: key even when the value is nil. Personally I prefer being explicit in these situations, but, anyway, I guess we must be consistent with the behaviour for meta keys (if there is still any at the end of the work in this PR).

solnic · 2019-01-02T08:25:58Z

I have redefined ROM::SQL::Function#name method in the repository
integration tests. This should be changed in rom-sql project. How would yo
go with it?

Please open a PR in rom-sql which changes this, preferably with tests. You could temporary target this branch in the rom-sql's Gemfile, once it's merged we can go back to depending on master.

core/lib/rom/schema.rb

core/lib/rom/schema/dsl.rb

waiting-for-dev · 2019-01-03T08:51:26Z

Please open a PR in rom-sql which changes this, preferably with tests. You could temporary target this branch in the rom-sql's Gemfile, once it's merged we can go back to depending on master.

Before I needed to push here the update to the schema DSL. I added some comments to the code to make it clear some intentions.

References rom-rb/rom#512

core/lib/rom/schema.rb

waiting-for-dev · 2019-01-03T16:38:59Z

I'm stuck with the last change needed in order to have the new feature available.

We need the following to work:

schema(infer: true) do
  attribute :first_name, alias: :name
end

Attribute inferrer checks whether an attribute has been already defined by the user comparing names. However, :name is an option in meta, which is something related with the type, and with this new DSL feature we would be defining the attribute options but not the type. Looks like we should also move :name to be an attribute option?

solnic · 2019-01-04T12:06:43Z

Looks like we should also move :name to be an attribute option?

Yes, this makes perfect sense. My understanding is that first and foremost we're moving attribute-related options to the new options hash, instead of type meta. So :name should be moved there too.

waiting-for-dev · 2019-01-04T12:59:40Z

rom/core/lib/rom/schema.rb

Lines 140 to 148 in a9a9f2d

    
           def self.attributes(attributes, attr_class) 
        
             attributes.map do |attr| 
        
               if attr.is_a?(Hash) 
        
                 attr_class.new(attr[:type], attr.fetch(:options, EMPTY_HASH)) 
        
               else 
        
                 attr_class.new(attr) 
        
               end 
        
             end 
        
           end

Do you think that backward compatibility issues with plugins could arise? Changing :name to be an option would make the attribute information extracted from the DSL to be always a Hash.

solnic · 2019-01-04T15:18:34Z

Changing :name to be an option would make the attribute information extracted from the DSL to be always a Hash.

Not sure if I follow, wdym by the attribute information extracted from the DSL?

waiting-for-dev · 2019-01-05T20:28:54Z

Not sure if I follow, wdym by the attribute information extracted from the DSL?

I pushed a commit with the changes. I'll try to explain it in a review, alongside other comments.

My understanding is that first and foremost we're moving attribute-related options to the new options hash, instead of type meta.

So, do you think we should move everything from meta to options? :primary_key, :source...

changeset/spec/spec_helper.rb

core/lib/rom/attribute.rb

core/lib/rom/schema.rb

waiting-for-dev · 2019-01-05T20:38:25Z

core/lib/rom/schema.rb

+    # @return [Hash] A hash with `:type` and `:options` keys.
+    #
+    # @api private
+    def self.build_attribute_info(type, options)


Defining this method here, because it seems that the attribute class is decoupled from the attribute representation (Schema.define accepts the attribute class as argument). Otherwise, maybe we could work with Attribute#ast, which would be neater.

core/spec/unit/rom/attribute/method_missing_spec.rb

waiting-for-dev · 2019-01-06T06:55:35Z

spec/sql_overrides.rb

+
+          if type
+            if args.empty?
+              ::ROM::SQL::Function.new(type, name: :not_used, schema: schema)


To be honest, not sure what is happening here. But seems that name: is not needed at all.

I'm specially interested in this line. I'm trying to adapt rom-sql to the new changes, and I see that attributes without a name are created in a lot of places. So, should we make :name optional?

:name is required, in case of functions it's just added later, because you create a function first, then you give it a name via #as, ie str::upper(:name).as(:first_name). This is not pretty from a design pov of course, because we should probably introduce an intermediate object that represents a wip-function, and its #as method would return the actual function. This is a refactoring that could be done later if we decide it's a good idea.

I see. I guess that in the meantime, before facing that refactor, the only possibility is to have :name as optional in its dry-initializer definition.

For now let's do this:

# FIXME: refactor so that it returns an intermediate function representation which # needs to a name ::ROM::SQL::Function.new(type, name: :__anonymous__, schema: schema)

I'll make sure this gets addressed before the release.

@solnic I was now re-adapting rom-sql to new changes, and I was adding name: :__anonymous everywhere a function is created without name. The problem is that AS "__anonymous__" gets added to a lot of resulting SQL queries.

Maybe we should tackle that refactor first... or just stick with nil names in the meanwhile.

Oh damn :/ My bad, apologies, I didn't think this through. Could you try redefining option :name, optional: true, type: Types::Symbol and see if that works?

waiting-for-dev · 2019-01-06T10:03:06Z

The previous commit added the end goal feature of being able to do:

schema(infer: true) do
  attribute :name, alias: :username
end

Besides any change required to the API or the implementation, it would just remain to be done moving other meta's to options, if we think it is a good idea.

core/lib/rom/attribute.rb

solnic · 2019-01-07T13:28:49Z

spec/sql_overrides.rb

+
+          if type
+            if args.empty?
+              ::ROM::SQL::Function.new(type, name: :not_used, schema: schema)


:name is required, in case of functions it's just added later, because you create a function first, then you give it a name via #as, ie str::upper(:name).as(:first_name). This is not pretty from a design pov of course, because we should probably introduce an intermediate object that represents a wip-function, and its #as method would return the actual function. This is a refactoring that could be done later if we decide it's a good idea.

solnic · 2019-01-08T08:46:00Z

Besides any change required to the API or the implementation, it would just remain to be done moving other meta's to options, if we think it is a good idea.

Let's move the rest in a separate PR. This is getting big enough already.

References rom-rb/rom#512

waiting-for-dev · 2019-01-13T08:23:35Z

Ok, I re-adapted rom-sql to work with :name being an (optional) option (pending refactor here about functions being created without a name).

Tell me if you want some other codeclimate offense to be addressed. About the merge_attributes method, I tried to optimize its performance so it is a bit obscure, but I think it pays off. However, one option to simplify it would be moving the type_lookup method as a helper class method in Attribute or Schema.

Also in this method and in general for all rom and rom-sql, I think that a very nice refactor would be to create an equivalent method to Attribute#with but working with params (type in this case). A lot of errors re-adapting for the changes aroused from calling #new to Attribute without copying both params and options. Even better, I think that #with and the new method should be created in dry-initializer.

Besides any other more than welcome code review, I think this PR is ready to go 😄

houndci-bot · 2019-04-04T15:17:44Z

core/spec/unit/rom/plugins/schema/timestamps_spec.rb

@@ -25,7 +25,7 @@

  it 'supports custom types' do
    schema_dsl.use :timestamps, type: ROM::Types::Date
-    dt_attribute = -> name { ROM::Attribute.new(ROM::Types::Date.meta(name: name, source: relation)) }
+    dt_attribute = -> name { ROM::Attribute.new(ROM::Types::Date.meta(source: relation), name: name) }


Metrics/LineLength: Line is too long. [102/100]

houndci-bot · 2019-04-04T15:17:45Z

core/spec/unit/rom/plugins/schema/timestamps_spec.rb

@@ -8,7 +8,7 @@
  subject(:schema) { schema_dsl.call }

  it 'adds timestamp attributes' do
-    ts_attribute = -> name { ROM::Attribute.new(ROM::Types::Time.meta(name: name, source: relation)) }
+    ts_attribute = -> name { ROM::Attribute.new(ROM::Types::Time.meta(source: relation), name: name) }


Metrics/LineLength: Line is too long. [102/100]

houndci-bot · 2019-04-04T15:17:45Z

core/spec/unit/rom/attribute/to_ast_spec.rb


  types.each do |type|
    specify do
-      expect(to_attr.(type).to_ast).to eql([:attribute, [:id, type.to_ast, {}]])
+      expect(to_attr.(type).to_ast).to eql([:attribute, [:id, type.to_ast, {alias: nil}]])


Layout/SpaceInsideHashLiteralBraces: Space inside { missing.
Layout/SpaceInsideHashLiteralBraces: Space inside } missing.

houndci-bot · 2019-04-04T15:17:45Z

core/spec/support/schema.rb

+          type: define_type(id, source: source),
+          options: { name: name }
+        }
+      end)


Layout/MultilineMethodCallBraceLayout: Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.

houndci-bot · 2019-04-04T15:17:46Z

core/spec/integration/relations/default_dataset_spec.rb

@@ -27,7 +27,7 @@
        insert(id: 2, name: 'Joe')
        insert(id: 1, name: 'Jane')

-        order(*rel_klass.schema.primary_key.map { |t| t.meta[:name] })
+        order(*rel_klass.schema.primary_key.map { |t| t.name })


Style/SymbolProc: Pass &:name as an argument to map instead of a block.

houndci-bot · 2019-04-04T15:17:48Z

repository/spec/spec_helper.rb

@@ -76,4 +78,4 @@ def warn(str)
  end

  config.include(MapperRegistry)
-end
+end


Layout/TrailingBlankLines: Final newline missing.

houndci-bot · 2019-04-04T15:17:48Z

core/spec/unit/rom/schema_spec.rb

+      {
+        user_id: define_attr_info(:Integer, { name: :user_id }, primary_key: true),
+        group_id: define_attr_info(:Integer, { name: :group_id }, primary_key: true),
+        name_id: define_attr_info(:String, name: :name ),


Layout/SpaceInsideParens: Space inside parentheses detected.
Style/TrailingCommaInHashLiteral: Avoid comma after the last item of a hash.

houndci-bot · 2019-04-04T15:17:49Z

core/spec/unit/rom/schema_spec.rb

-                  [[:attribute, [:id, [:nominal, [Integer, {}]], {}]],
-                   [:attribute, [:name, [:nominal, [String, {}]], {}]]]]])
+                  [[:attribute, [:id, [:nominal, [Integer, {}]], {alias: nil}]],
+                   [:attribute, [:name, [:nominal, [String, {}]], {alias: nil}]]]]])


Layout/SpaceInsideHashLiteralBraces: Space inside { missing.
Layout/SpaceInsideHashLiteralBraces: Space inside } missing.
Layout/MultilineArrayBraceLayout: Closing array brace must be on the line after the last array element when opening brace is on a separate line from the first array element.

houndci-bot · 2019-04-04T15:17:49Z

core/spec/unit/rom/schema_spec.rb

      schema = ROM::Schema.define(:name, attributes: attrs.values)

      expect(schema.to_ast).
        to eql([:schema, [
                  :name,
-                  [[:attribute, [:id, [:nominal, [Integer, {}]], {}]],
-                   [:attribute, [:name, [:nominal, [String, {}]], {}]]]]])
+                  [[:attribute, [:id, [:nominal, [Integer, {}]], {alias: nil}]],


Layout/SpaceInsideHashLiteralBraces: Space inside { missing.
Layout/SpaceInsideHashLiteralBraces: Space inside } missing.

houndci-bot · 2019-04-04T15:17:49Z

core/spec/unit/rom/schema_spec.rb

      schema = ROM::Schema.define(:name, attributes: attrs.values)

-      expect(schema.to_h).to eql(attrs)
+      expect(schema.to_h).to eql({ id: ROM::Attribute.new(ROM::Types::Integer, name: :id),


Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.

houndci-bot · 2019-04-17T10:03:54Z

core/spec/unit/rom/schema/wrap_spec.rb

@@ -11,7 +11,7 @@

  it 'returns projected schema with renamed attributes using provided prefix' do
    expect(wrapped.map(&:alias)).to eql(%i[users_id users_name])
-    expect(wrapped.map { |attr| attr.meta[:name] }).to eql(%i[id name])
+    expect(wrapped.map { |attr| attr.name }).to eql(%i[id name])


Style/SymbolProc: Pass &:name as an argument to map instead of a block.

houndci-bot · 2019-04-17T10:03:54Z

core/spec/unit/rom/schema/prefix_spec.rb

@@ -11,6 +11,6 @@

  it 'returns projected schema with renamed attributes using provided prefix' do
    expect(prefixed.map(&:alias)).to eql(%i[user_id user_name])
-    expect(prefixed.map { |attr| attr.meta[:name] }).to eql(%i[id name])
+    expect(prefixed.map { |attr| attr.name }).to eql(%i[id name])


Style/SymbolProc: Pass &:name as an argument to map instead of a block.

houndci-bot · 2019-04-17T10:03:54Z

core/spec/unit/rom/schema/finalize_spec.rb

@@ -30,12 +30,13 @@
    end

    let(:attributes_inferrer) do
-      proc { [ [define_attribute(:name, :String)], %i(id age) ] }
+      proc { [ [define_attribute(:String, name: :name)], %i(id age) ] }


Layout/SpaceInsideArrayLiteralBrackets: Do not use space inside array brackets.
Style/PercentLiteralDelimiters: %i-literals should be delimited by [ and ].

houndci-bot · 2019-04-17T10:03:54Z

core/spec/unit/rom/schema/attribute_spec.rb

+      end
+
+      specify do
+        expect(attr.inspect).to eql("#<ROM::Attribute[TrueClass | FalseClass] name=:admin alias=:adm>")


Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
Metrics/LineLength: Line is too long. [103/100]

houndci-bot · 2019-04-17T10:03:54Z

core/spec/unit/rom/schema/attribute_spec.rb

+      end
+
+      specify do
+        expect(attr.inspect).to eql("#<ROM::Attribute[TrueClass | FalseClass] name=:admin alias=nil>")


Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
Metrics/LineLength: Line is too long. [102/100]

houndci-bot · 2019-04-17T10:03:56Z