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

Reset tenant #5

Merged
merged 1 commit into from
Feb 24, 2022
Merged

Reset tenant #5

merged 1 commit into from
Feb 24, 2022

Conversation

i-am-gdan
Copy link

@i-am-gdan i-am-gdan commented Jan 27, 2022

Hi!
I was having 2 issues with small fixes so I thought I'd make a PR rather than an issue:

  1. when I was switching between a subdomain, no subdomain e.g. test.relished.co to relished.co where the test.relished.co data was showing up on relished.co. I noticed that if there is no subdomain the tenant is not reset
  2. The fetch method was failing for me.

Let me know what you think! @Dandush03

@i-am-gdan i-am-gdan changed the base branch from master to development January 27, 2022 16:49
Copy link
Owner

@Dandush03 Dandush03 left a comment

Choose a reason for hiding this comment

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

I love seeing that you're contributing to this project 😃, took me some time to get it decent enough to share it 😅

My next upgrade to this gem was to write some custom error can we instead of your suggestion, add a custom error for no tenant e.g.;

First File

# lib/pg_rls.rb
....
require_relative 'pg_rls/errors'
....
# remove line 16
....

Second File

module PgRls
  module Errors
    class MissingTenant < StandardError
      def initialize
        reset_tenant_id
        super
      end
      
      def message
        "Tenant Doesn't exist"
      end
      
      def reset_tenant_id
        PgRls.connection_class.connection.execute('RESET rls.tenant_id')
      end
    end
  end
end

and then we can implement it on PgRls::Tenant or something like this. Let me know your thought

NOTE: haven't tested it, feel free to rename the path and error name :)

@i-am-gdan
Copy link
Author

I've updated from your suggestions - if this looks good, let me know and I'll rebase to squash it all together, just wanted to leave it like this for now so we can follow the history!

@@ -18,7 +18,7 @@ def switch(resource)
attr_reader :tenant

def fetch
@fetch ||= tenant.find_by_tenant_id(
Copy link
Author

Choose a reason for hiding this comment

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

I found that @fetch ||= was caching the value of fetch across different requests, so I've removed it, what do you think about the performance implications of this?

Copy link
Owner

Choose a reason for hiding this comment

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

So, originally my idea was to cache, the reason behind it is that when a user called multiple times it tenant_model.current would only look for that tenant once, Although I see that, that, could lead to some issue if you trying to switch between tenant under the same connection, to tackle that issue we could add a reset for this action on the switch method. Make sense? I'll look forward to your opinion on this matter, maybe I'm missing something

For example, Let's say you have a car controller and you want to load the current tenant preferences on time_zone. you can use TenantModel.current.time_zone and then on your wheel controller you want to know if this tenant allows the user to send emails for some crazy reason so you would call TenantModel.current.send_email? and it would not run duplicate queries.

It's the same as device caching the current_user, we could implement a different method and called it current_tenant and use it as an extended concern, but it would be a lot more code since we most adapt the 'tenant' to be eq to the resource given eg; current_admin or current_company

Copy link
Author

Choose a reason for hiding this comment

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

That's a nice solution! I'll make the change and test it!

Copy link
Owner

@Dandush03 Dandush03 left a comment

Choose a reason for hiding this comment

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

I think we are very close to wrap this up 😄 thanks for all your help

rescue NoMethodError => e
@error = e
rescue ActiveRecord::RecordNotFound
raise PgRls::Errors::TenantNotFound.new
Copy link
Owner

Choose a reason for hiding this comment

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

why do we most initialize it?

raise called method to initialize as far as I remember, if not we can change it a bit of instead of using the initializer method we could replace it with the message method and nest there the reset code eg;

module PgRls
  module Errors
    class TenantNotFound < StandardError
      def message
        reset_tenant_id
        "Tenant Doesn't exist"
      end

      def reset_tenant_id
        PgRls.connection_class.connection.execute('RESET rls.tenant_id')
      end
    end
  end
end

But I'm almost sure we don't require the new method, did you test it?

@@ -18,7 +18,7 @@ def switch(resource)
attr_reader :tenant

def fetch
@fetch ||= tenant.find_by_tenant_id(
Copy link
Owner

Choose a reason for hiding this comment

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

So, originally my idea was to cache, the reason behind it is that when a user called multiple times it tenant_model.current would only look for that tenant once, Although I see that, that, could lead to some issue if you trying to switch between tenant under the same connection, to tackle that issue we could add a reset for this action on the switch method. Make sense? I'll look forward to your opinion on this matter, maybe I'm missing something

For example, Let's say you have a car controller and you want to load the current tenant preferences on time_zone. you can use TenantModel.current.time_zone and then on your wheel controller you want to know if this tenant allows the user to send emails for some crazy reason so you would call TenantModel.current.send_email? and it would not run duplicate queries.

It's the same as device caching the current_user, we could implement a different method and called it current_tenant and use it as an extended concern, but it would be a lot more code since we most adapt the 'tenant' to be eq to the resource given eg; current_admin or current_company

@@ -9,6 +9,7 @@ def switch(resource)
find_tenant(resource)
connection_adapter.connection.execute(format('SET rls.tenant_id = %s',
connection_adapter.connection.quote(tenant.tenant_id)))
@fetch = nil
Copy link
Owner

Choose a reason for hiding this comment

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

Last comment sorry for being picky but since this won't have any test created yet I must be careful with our changes 😓, if you can make this change this weekend I'll approve it and we can merge it, so I can generate a new version :) BTW, can you change the version so it would include your changes 😉

Please move this to the top, so if anything fails we will guarantee the @fetch was reseted

# lib/pg_rls/version.rb

module PgRls
  VERSION = '0.0.1.4.2'
end

sorry I fail to mention this before

Previously fetch was throwing an error as it was calling
find_by_tenant_id on an instance of tenant. This should make the
method more flexible

Reset Tenant if tenant id not present in request
This is to ensure that tenant is reset to null if a request with no
tenant is made to an instance where tenant has previously been set.
Copy link
Owner

@Dandush03 Dandush03 left a comment

Choose a reason for hiding this comment

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

I Really Appreciate Your Contribution 😃

@Dandush03 Dandush03 merged commit 17a7da2 into Dandush03:development Feb 24, 2022
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.

2 participants