amazon-archives / aws-flow-ruby

ARCHIVED
136 stars 57 forks source link

Unable to create Workflows object inside of Module #32

Closed mtrimmer closed 10 years ago

mtrimmer commented 10 years ago

If I try to declare my workflow class inside of a module, I get a Invalid Name error when the Flow lib tries to register the workflow type with '::'s in the name.

module Test
  class HelloWorldWorkflow
    extend AWS::Flow::Workflows

    # Create a workflow instance, passing the workflow method name and providing
    # some option values.
    workflow(:hello_workflow) do | opts |
      opts.version = "1"
      opts.task_list = $TASK_LIST
      opts.execution_start_to_close_timeout = 3600
    end

    # Define the workflow that was passed to the AWS::Flow::Workflows#workflow
    # method.
    def hello_workflow(name)
      activity.hello_activity(name)
    end

    # Create an activity client to
    activity_client(:activity) {{ :from_class => "HelloWorldActivity" }}

  end
end
# Create an workflow worker that can start the workflow.
workflow_worker = AWS::Flow::WorkflowWorker.new(@swf.client, @domain, $TASK_LIST, Test::HelloWorldWorkflow)

# Start the workflow worker if this file is run directly, such as:
#
#    ruby hello_workflow.rb &
#
workflow_worker.start if __FILE__ == $0

I get the following exception:

/var/lib/gems/1.9.1/gems/aws-sdk-1.32.0/lib/aws/core/client.rb:368:in `return_or_raise': Invalid name: Test::HelloWorldWorkflow.hello_workflow (AWS::SimpleWorkflow::Errors::ValidationException)
        from /var/lib/gems/1.9.1/gems/aws-sdk-1.32.0/lib/aws/core/client.rb:469:in `client_request'
        from (eval):3:in `register_workflow_type'
        from /var/lib/gems/1.9.1/gems/aws-flow-1.0.8/lib/aws/decider/worker.rb:174:in `block in register'
        from /var/lib/gems/1.9.1/gems/aws-flow-1.0.8/lib/aws/decider/worker.rb:172:in `each'
        from /var/lib/gems/1.9.1/gems/aws-flow-1.0.8/lib/aws/decider/worker.rb:172:in `register'
        from /var/lib/gems/1.9.1/gems/aws-flow-1.0.8/lib/aws/decider/worker.rb:197:in `start'
        from ./hello_workflow.rb:36:in `<main>'

This appears to be caused by the '::' in the name, which are invalid characters for the workflow name.

It seems like we might want to sanitize the names to allow them to be declared inside of a module.

mtrimmer commented 10 years ago

It seems I get get around this be explicitly setting the option prefix_name to something that does not include any illegal characters, but it would be nice if the default behavior didn't die when using a class w/in a module.

mjsteger commented 10 years ago

Setting the prefix_name is indeed the current recommended workaround. I'll see if I can warn out earlier if illegal characters are included, as the current error is non-obvious and doesn't let the user know about the "correct" way to do things. Sorry for the trouble.

rantonmattei commented 10 years ago

Hi,

I'm kind of reopening this issue. I reached out to the aws-sdk-ruby team but they pointed me back to the Flow repo: see last comment from @trevorrowe at https://github.com/aws/aws-sdk-ruby/issues/581#issuecomment-49637197

I don' think using a prefix really helps. The issue is that we cannot use ":" as a Workflow name. Couldn't Flow do a quick character replace before registering the workflow and replace it back when retrieving the workflow type from SWF? It is a bit hacky but that could be a workaround.

Essentially, it is the same issue. Here is my original message below:

If my workflow class is part of a namespace like shown below, I get the following error: ruby-2.1.2/gems/aws-sdk-1.42.0/lib/aws/core/client.rb:375:in `return_or_raise': Invalid name: MyWorkflows::HelloWorldWorkflow.hello (AWS::SimpleWorkflow::Errors::ValidationException)

A quick look to the doc @ http://docs.aws.amazon.com/amazonswf/latest/apireference/API_RegisterWorkflowType.html confirms that the RegisterWorkflowType API action cannot take a name with a colon.

The specified string must not start or end with whitespace. It must not contain a : (colon), / (slash), | (vertical bar), or any control characters (\u0000-\u001f | \u007f - \u009f). Also, it must not contain the literal string "arn".


So that raises at least 2 main concerns:


require_relative "../../swf_init"
require_relative "activities/hello"
# HelloWorldWorkflow class defines the workflows for the HelloWorld sample
module MyWorkflows
  class HelloWorldWorkflow
    extend AWS::Flow::Workflows
    workflow :hello do {
      version: HelloWorldInit::WF_VERSION,
      task_list: HelloWorldInit::WF_TASKLIST,
      execution_start_to_close_timeout: 3600,
    }
    end
    # Create an activity client using the activity_client method to schedule
    # activities
    activity_client(:hello_client) { { :from_class => "HelloActivity" } }
    # This is the entry point for the workflow
    def hello(name)
      # Use the activity client 'client' to invoke the say_hello activity
      hello_client.say_hello(name)
    end
  end
end
# Start a WorkflowWorker to work on the HelloWorldWorkflow tasks
HelloWorldInit.new.workflow_worker(MyWorkflows::HelloWorldWorkflow).start if $0 == __FILE__
# HelloWorldInit.new.workflow_worker eventually calls
# AWS::Flow::WorkflowWorker.new(domain.client, domain, WF_TASKLIST, workflow_class)
# workflow_class being MyWorkflows::HelloWorldWorkflow

pmohan6 commented 10 years ago

Can you explain how setting the prefix name didn't work for you? It is the recommended way of getting around the issue. If you set the prefix_name with a string that doesn't contain illegal characers, ruby flow will register the workflow with the name "prefix_name.". So if you change your workflow implementation to the following -

module MyWorkflows
  class HelloWorldWorkflow
    extend AWS::Flow::Workflows

    workflow :hello do {
      version: HelloWorldInit::WF_VERSION,
      task_list: HelloWorldInit::WF_TASKLIST,
      execution_start_to_close_timeout: 3600,
      prefix_name: "MyWorkflows.HelloWorldWorkflow" # <------ add the prefix name
    }
    end

    # Create an activity client using the activity_client method to schedule
    # activities
    activity_client(:hello_client) { { :from_class => "HelloActivity" } }

    # This is the entry point for the workflow
    def hello(name)
      # Use the activity client 'client' to invoke the say_hello activity
      hello_client.say_hello(name)
    end
  end
end

ruby flow will try to register the workflow with SWF with the name 'MyWorkflows.HelloWorldWorkflow.hello', hence getting rid of the colons.

rantonmattei commented 10 years ago

Thanks for your response.

Flow actually explicitly prevents you from using "." in prefix_name. I get the following error:

.../.rvm/gems/ruby-2.1.2/gems/aws-flow-1.2.0/lib/aws/decider/options.rb:172:in `block in class:Options': You cannot have a . in your prefix name (RuntimeError)

Check out @ https://github.com/aws/aws-flow-ruby/blob/master/aws-flow/lib/aws/decider/options.rb#L172

Ideally, I'd like to register something like "MyWorkflows::HelloWorldWorkflow.hello" but this is not allowed by SWF because of the ":".

The only workaround I see is a character replacement in the SDK or in Flow unless the SWF folks allow ":" directly ;-)

pmohan6 commented 10 years ago

Sorry, I missed the '.'. We will consider adding a feature to escape ":" in future versions of aws-flow. In the meantime, would it suffice for you to set the prefix_name with a string that doesn't contain any of the restricted characters?

rantonmattei commented 10 years ago

Well, I flattened the design for now so my workflow classes are at the top level of the namespace. It is working because the classname does not contain any namespace separator anymore (i.e. "::"). Prefix name does not really help at the moment.

Let me know whenever you guys implement this. Thanks again for your help!

pmohan6 commented 10 years ago

The task is in our backlog. I'll update this issue when we do release it. Resolving it for now. Thanks!