Decouple attribute alias from type definition

[waiting-for-dev]

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]

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]

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 :slightly_smiling_face:

[solnic]

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]

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]

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

[waiting-for-dev]

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]

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.

[waiting-for-dev]

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.

[waiting-for-dev]

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]

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]

https://github.com/rom-rb/rom/blob/a9a9f2d1676e2a48a1ec94c8b85e97c9c162793d/core/lib/rom/schema.rb#L140-L148

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]

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]

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

[waiting-for-dev]

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.

[solnic]

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.

[waiting-for-dev]

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 :smile:

[solnic]

@waiting-for-dev thanks again for working on this. I rebased it and cleaned up commits. I'll merge once CI is green.

[waiting-for-dev]

Hey @solnic I was going to tackle CI offenses right now, but it seems you just merged it at the same exact time?

[solnic]

@waiting-for-dev feel free to push such fixes directly to master

[waiting-for-dev]

Ok @solnic , thanks for your feedback and your support with this PR.