-
Notifications
You must be signed in to change notification settings - Fork 60
use current_schema in PLSQL::Schema#schema_name #94
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
Conversation
|
I used alter session set current_schema a lot at my previous project as we were using multi-schema deployments on single dev instance to avoid stepping on each other's toes when developing. So each developer/feature had his schema to play with. |
|
From what I remember. We were using |
|
This change looks good. |
|
Looks good to me. It may happen that in some contexts the session user would be needed, while in others - current schema.
It's good to have tests. I think we already have 2 test schema |
|
You're right, no reason not to write tests. Done! |
spec/plsql/package_spec.rb
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That got me thinking about an edge case - what if we connect through proxy user? (I'm not sure whether the other tests would be compatible with such scenario either).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That got me thinking about an edge case - what if we connect through proxy user? (I'm not sure whether the other tests would be compatible with such scenario either).
Looks like the other tests already expect the username used to connect to match session user:
schema_spec.rb#L37.
So I think it's OK to rely on this assumption for now. We may think of how to address proxy user connections some other time in future.
|
Thanks for your feedback @javornikolo, I updated tests to be more descriptive (hopefully!). Regarding proxy users and a setter for schema_name: the specs frequently reset connections just for the side effect of clearing cached metadata, which was confusing to me. oracle-enhanced sets current_schema right away on new connections so it is a relatively simple use case, but edge cases like those might also be easier to implement if the PLSQL::Schema API was more transparent about what it is caching (and how and when to reset it). |
spec/plsql/package_spec.rb
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if would be better to have after(:all) close to the relevant before block (it feels a bit easier to see what we need to tear down).
|
Good spot about the caching... It may be cleaner to add a schema_name setter which takes care about purging the cached stuff. And still - looks like we're still in trouble when current_schema is changed by a database call, resetting the cache via |
|
I agree that that's the way to go, but I think it's a separate issue and deserves its own PR. This is a surgical fix to get the gem working like I'd expect with oracle-enhanced 1.6, messing with caching is more tricky. Re: the after block, you're right that this project usually has |
|
Cool, it looks good enough to me now :-) I agree - the other intricacies can be handled in separate PR. @substars, thank you very much for this contribution! |
use current_schema in PLSQL::Schema#schema_name
We're using oracle-enhanced with schema: parameter set, so current_schema is different than the current user, which causes PL/SQL not to find available packages. current_schema should (?) be the same as session_user unless explicitly set otherwise, in which case we should use that.
I'm happy to write a specific test for this, but it'll make setup more complex since we'll need to create another user, do some grants, etc. Thoughts?