dandemeyere / responsys-api

MIT License
20 stars 19 forks source link

Allow to create multiple instances of Responsys::Api::Client #6

Open zhuravel opened 10 years ago

zhuravel commented 10 years ago

Hi guys,

First off thank you for this gem, absolute lifesaver! :)

A great feature would be the ability to create multiple instances of Client class with different credentials. Right now it is a singleton which unfortunately doesn't work for me and may not work for other gem users who need to call more than one Responsys API in one application.

dandemeyere commented 10 years ago

Hi @zhuravel,

The reason why it's a singleton right now is because we didn't want to have to authenticate for every API request. We wanted to authenticate once with our credentials and then use those credentials for all subsequent API calls until we close the connection. We only have 1 interact account, does your application deal with multiple Responsys Interact accounts?

zhuravel commented 10 years ago

@dandemeyere Yes, I need to hit 3 different Responsys Interact APIs. I think I wouldn't have to authenticate for every API request with code like this:

class ResponsysAccounts
  class_attribute :instances
  self.instances = {}

  def self.set(identifier, client)
    self.instances[identifier] = client
  end

  def self.get(identifier)
    self.instances[identifier]
  end
end

ResponsysAccounts.set(:first, Responsys::Api::Client.new(username: ..., password: ..., wsdl: 'xxxx'))
ResponsysAccounts.set(:second, Responsys::Api::Client.new(username: ..., password: ..., wsdl: 'yyyy'))

client = ResponsysAccounts.get(:first)
client.api_method(:list_folders)
dandemeyere commented 10 years ago

If you require the ability to handle multiple Responsys accounts in the near future, I would suggest either forking our gem or opening a pull request containing the code to add that functionality so we can merge it in (assuming you add tests, documentation, etc.).

We're still in the onboarding phase with Responsys and we have a fair amount of functionality to build out before our integration is complete so the priority for our efforts is to build this gem such that our business (thredUP) is able to utilize Responsys Interact API functionality as soon as possible. After we have a stable version of the gem that we're confident in and we've completed our integration with Responsys, then we'll come back and think through what the next steps are for this gem. I don't have a timetable for when that would be, but we can keep this issue open and we can re-visit this topic once we reach that point.

atomical commented 8 years ago

Hi @dandemeyere,

Is the client thread-safe?

dandemeyere commented 8 years ago

@florrain - can you help answer that question when you have time please?

florrain commented 8 years ago

@atomical - The client has never been inspected or thought to be thread-safe at any point.

Looking at the code, the section that might come into problems with different threads is the configuration of the GEM (Responsys::Configuration ||= operation for ex) and the initialization of the SessionPool. If you're trying to use multiple accounts at the same time for example, I'm worried that the Configuration won't be safe because of that and the way the Configuration is used within the GEM.

I suggest you double check in the code, especially if you're worried about this kind of issue you might be aware of good/bad safe patterns. Any suggestions/PR will be appreciated to make the client thread-safe if it isn't enough today.