Add Configuration#multi_tenant! for opting into multi-tentant support where configuration is per-thread

[cgunther]

This is a follow up to #549 to improve usage in a single tenant scenario where you might configure NetSuite on the main thread (ie. via Rails initializer), then make requests in another thread (ie. Sidekiq jobs).

By default we assume single-tenant usage and the configuration is shared across all threads. If you opt into multi-tenant usage, each child thread starts with the main thread's configuration, but can then be further configured without altering other threads.

Rather than mixing Thread.current and global instance variables, I'm using Thread.main for the global configuration.

Child threads start with a copy of the main thread's attributes, figuring this still allows you to set some things globally on the main thread (ie. timeouts), then per-tenant things on the child threads (ie. account).

Whether we're multi-tentant or not is always stored on the main thread, not as an attribute, as we want/need that to be global.

One possible concern is what happens if you set some configuration before calling multi_tenant!, particularly when called from a child thread. Whatever configuration you set before multi_tenant! would be stored on the main thread (thinking we're single-tenant), not the child thread, which could be surprising. Maybe we need a safety check so it'll error if you call multi_tenant! after attributes have already been set, forcing you to specify multi_tentant! first?

reset! also resets whether it's multi-tenant, taking the stance that it's a full reset, not just resetting the thread.

I didn't touch caches yet, wanted to make sure we're on the right path first. I'll need to mention this in the README too.

[iloveitaly]

Thank you for the great contribution! Left some comments.

[cgunther]

Thanks for the feedback, took that into account.

I followed the same pattern with attributes to apply to the caches that were changed in #549.

Also documented the configuration in the README.

I'm still unsure of whether Configuration#reset! should reset multi-tenancy or not. All the examples in the README start with a reset!, which seems unnecessary, at least in a single-tenant scenario, which is the extent of my experience. Trying to speculate on the multi-tenant scenario, if you're going to start your configuration in each child thread with a reset!, then you'd also need to follow it with a multi_tenant! call since the reset currently resets back to single-tenant mode. Broadly, I think we need to decide if multi-tenancy is a configuration (just like account, api_version, etc.), such that it's resettable, or is it a new higher-level concept that is not resettable (ie. NetSuite.multi_tenant!), and thus the configuration is resettable, but that doesn't change the multi-tenancy.

[iloveitaly]

Broadly, I think we need to decide if multi-tenancy is a configuration (just like account, api_version, etc.), such that it's resettable, or is it a new higher-level concept that is not resettable (ie. NetSuite.multi_tenant!), and thus the configuration is resettable, but that doesn't change the multi-tenancy.

I think this is the right framing of the problem. My thought is multi_tenant! should be a separate concept (maybe with it's own instance var?) which is not effected by reset!.

That is a bit weird, but having it effected by reset! would be worse.

Wdyt?

[cgunther]

I tweaked things so reset! no longer resets multi-tenancy (it's stored as it's own instance variable, not within the attributes hash) and updated the README to reflect that you'd probably configure once in your main thread to enable multi-tenancy, then configure again in each child thread to set your other attributes.

The test gets a little wonky as we have to use instance_variable_set to reset multi-tenancy after enabling it to ensure it doesn't leak to other tests, as we don't publicly offer a way to disable multi-tenancy.

It slightly irks me that #multi_tenant! lives in Configuration since it's really at a higher level, but there's a smidge of precedent for non-attribute methods in this class (ie. #connection, #wsdl_cache, etc.) so maybe it's ok for now.

Ultimately I'd really like feedback from someone with a multi-tenant setup, as my experience is only with single-tenant setups, so I'm just speculating on how a multi-tenant setup might look. My motivation with this PR was to address that #549 broke my single-tenant set up.

[iloveitaly]

@cgunther Awesome work! I have a multi-tenant setup. Going to merge this in and run it against our internal test suite.

[cgunther]

Thanks for the merge.

Things seem to work well for my single-tenant setup. Assuming they work well for your multi-tenant setup, it might be wise to release a new version. I suspect the current version (v0.9.0) would be broken for anyone with a single-tenant setup, but using threads (ie. puma, sidekiq) and configuring outside those child threads as a result of #549.

[iloveitaly]

@cgunther definitely! I'll get a release out soon :)