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

Verify origins do not collide with ServiceActor::Result object methods #138

Conversation

viralpraxis
Copy link
Contributor

@viralpraxis viralpraxis commented Mar 4, 2024

Resolves #137

I'm not sure about removing display-related tests and code, but since we do not allow these methods in inputs/outputs seems like we dont have to be specific about display

Also added some tests and removed lonely require ostruct

> irb(main):004:0> RUBY_VERSION
> => "3.0.6"
> irb(main):005:0> ServiceActor::Result.instance_methods.to_s
> => "[:merge!, :key?, :inspect, :failure?, :delete, :[], :[]=, :fail!, :success?, :to_h, :pretty_print_instance_variables, :pretty_print_cycle, :pretty_print, :pretty_print_inspect, :taint, :tainted?, :untaint, :untrust, :untrusted?, :trust, :methods, :singleton_methods, :protected_methods, :private_methods, :public_methods, :instance_variables, :instance_variable_get, :instance_variable_set, :instance_variable_defined?, :remove_instance_variable, :instance_of?, :kind_of?, :is_a?, :gem, :class, :frozen?, :then, :public_send, :method, :public_method, :singleton_method, :tap, :define_singleton_method, :extend, :clone, :yield_self, :pretty_inspect, :to_enum, :enum_for, :<=>, :===, :=~, :!~, :nil?, :eql?, :respond_to?, :freeze, :object_id, :send, :to_s, :display, :hash, :singleton_class, :dup, :itself, :!, :==, :!=, :equal?, :instance_eval, :instance_exec, :__id__, :__send__]"

@viralpraxis viralpraxis marked this pull request as ready for review March 4, 2024 17:47
@viralpraxis viralpraxis marked this pull request as draft March 5, 2024 14:43
@viralpraxis
Copy link
Contributor Author

viralpraxis commented Mar 8, 2024

I tweaked implementation and not the total list of "dangerous" methods is much shorter:

# ServiceActor::Result.instance_methods
:merge!
:send
:__delete__
:key?
:pretty_print
:failure?
:inspect
:fail!
:class
:success?
:[]=
:[]
:kind_of?
:is_a?
:respond_to?
:to_h
:equal?
:!
:__send__
:==
:!=
:__binding__
:instance_eval
:instance_exec
:__id__

But one issue still remains, using these symbols in inputs/outputs/aliases will trigger the warning AND will
produce unexpected result in runtime:

[7] pry(main)> klass = Class.new(Actor) { input :inspect };
DEPRECATED: Defining inputs, outputs or alias_input that collide with `ServiceActor::Result` instance methods will lead to runtime errors in the next major release of Actor. Problematic input: `inspect`
[8] pry(main)> klass.call(inspect: 1).inspect
=> "<ServiceActor::Result {:inspect=>1}>"

In latest release (3.7.0) inspect would have been redefined and ServiceActor::Result#inspect would return 1.

cc @sunny

lib/service_actor/result.rb Outdated Show resolved Hide resolved
lib/service_actor/result.rb Outdated Show resolved Hide resolved
@sunny
Copy link
Owner

sunny commented Mar 10, 2024

I tweaked implementation and not the total list of "dangerous" methods is much shorter

That is awesome. 😍 Kudos on using a BasicObject yet adding the basic methods it would need.

But one issue still remains, using these symbols in inputs/outputs/aliases will trigger the warning AND will produce unexpected result in runtime

Oh, you’re right. So perhaps this should simply raise an ArgumentError straight away as the current behavior is already impacter. Now that the list of methods is smaller I don’t think we need a warning here.

@viralpraxis viralpraxis marked this pull request as ready for review March 10, 2024 20:00
@sunny sunny merged commit a7a64a3 into sunny:main Mar 10, 2024
7 checks passed
@sunny
Copy link
Owner

sunny commented Mar 10, 2024

Excellent! Not only does this warn of collisions it actually reduces it thanks to the SimpleObject. 👏🏻

@viralpraxis
Copy link
Contributor Author

@sunny I encountered a specific but crucial bug related to this PR. Gonna try to fix it soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Actor's output names collision with Object's instance methods
3 participants