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

Command plugin to accept input with aliased keys #517

Closed
wants to merge 6 commits into from

Conversation

waiting-for-dev
Copy link
Collaborator

@waiting-for-dev waiting-for-dev commented Jan 2, 2019

This PR adds an :alias command plugin that allows to use the alias for aliased attributes as input keys:

class User < ROM::Relations[:sql]
  schema(infer: true) do
    attribute :first_name, Types::String.meta(alias: name)
  end
end

class CreateUser < ROM::Commands::Create[:sql]
  result :one
  use :alias
end

result = rom.command(:user).create.call(name: 'Jane')
result[:first_name]  #=> 'Jane'

In my view, this can fix the current asymmetry between reading/writing data to/from the datastore. Now, when you configure an alias for an attribute, reading methods (like #first or #to_a), automatically perform the configured translation and return keys with the aliased names. However, when the user comes back to the engine with a command, that automation is lost and the "unwrapping" translation has to be handled manually. I see it as an abstraction leak, where the application level suddenly must be aware of canonical names (besides schema configuration, of course).

The good news are that it should be 100% backwards compatible, because commands using :alias plugin would still accept the canonical names as keys. So what do you think about activating this plugin by default? We could just check whether the schema has any alias at all and, if not, do not enter the tuple iteration for better performance.

In my limited knowledge of rom behaviour, I'm not sure if it is needed to test the plugin for different command scenarios. If so, please tell me.

Tests and documentation must be updated if #512 is merged.

Thanks.

Resources

@waiting-for-dev
Copy link
Collaborator Author

I have given it a second thought. Maybe instead of a command plugin, it would make more sense for it to be a relation command overloading insert, update relation methods if they are present in the adapter. I guess that commands are calling them internally, and it seems the actual application/database layer boundary.

@solnic
Copy link
Member

solnic commented Jan 11, 2019

This should be a command plugin. In the future, commands will be decoupled from relations, and will work with datasets directly. Relation insert and update are meant to stay bare-bones, as they are simple building blocks for other APIs.

@solnic solnic added the feature label Mar 27, 2019
@solnic solnic added this to the 5.0.0 milestone Mar 27, 2019
@solnic solnic removed this from the 5.0.0 milestone Apr 18, 2019
@solnic solnic added this to the 5.1.0 milestone Apr 18, 2019
@solnic
Copy link
Member

solnic commented Apr 18, 2019

@waiting-for-dev I'm sorry but I won't be able to work on finalizing this for 5.0.0 but we can pick it up quickly right after 5.0.0 is released.

@waiting-for-dev
Copy link
Collaborator Author

No worries @solnic . In the meantime I'm using a custom plugin in my project. However, let me know anything I can do.

@solnic
Copy link
Member

solnic commented Apr 28, 2019

@waiting-for-dev 👋 would you like to wrap this up for 5.1.0 release?

@@ -0,0 +1,36 @@
require 'rom/command'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.

klass.include(InstanceMethods)
end

module InstanceMethods

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/Documentation: Missing top-level module documentation comment.

#
# @api public
module Alias
module T

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/Documentation: Missing top-level module documentation comment.

@@ -0,0 +1,51 @@
require 'set'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.

@waiting-for-dev
Copy link
Collaborator Author

@solnic for now I rebased to master and adapted it to work with alias now being an option and not part of the meta.

But I think we have to give it some thoughts before merging.

Related to #524 , maybe we can 100% hide original database field names in the schema layer. If we make #[] not recognize original names which has been aliased, I think the command plugin defined here should neither recognize them. For the same reason, I think this plugin should be activated by default or even not being a plugin but part of the core behavior.

What do you think?

@solnic solnic added the wip label Jul 26, 2019
@solnic solnic changed the title [WIP] Command plugin to accept input with aliased keys Command plugin to accept input with aliased keys Jul 26, 2019
@solnic solnic modified the milestones: 5.1.0, 5.2.0 Jul 29, 2019
@solnic solnic modified the milestones: 5.2.0, 6.0.0 Jan 2, 2020
@solnic
Copy link
Member

solnic commented Jan 2, 2020

FYI we'll be releasing 5.2.0 with the primary goal to support ruby 2.7 and there's no time to finish this one for that release. I scheduled it to be included in 6.0.0 release instead.

@solnic
Copy link
Member

solnic commented Jun 24, 2020

I'm gonna close this because it needs more thinking/discussions. ie at the moment I think it should be a built-in behavior, we can apply mapping if there are aliased attributes automatically. I'll start an alias-related thread on the forum to discuss how to approach aliasing in 6.0.0.

@solnic solnic closed this Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants