Don't change the session office for information retrieval.

[MikeNeilson]

Actually going to try another fix, but just using the configuration directly caused the connection to stay open. Wanted to get this in due to other timing.

But in short the issue is that during the retrieval of isVersioned the connection session is set to the data set office when it doesn't need to be. So going to try the connectionResult in a different way to see if I can get it to behave.

[MikeNeilson]

getTimeZoneId looks like it could have the same issue.

[MikeNeilson]

Okay, so we have two choices here:

1. rely on the read of these values being open to everyone (e.g. except these changes)
2. Keep the new behavior and explicitly grant permissions to the HQ office to read things

Long term 2 may be better, but I don't think we're there yet.

[rma-rripken]

I don't understand why using the configuration directly would leave the connection open. That doesn't make sense to me.

[rma-rripken]

Do you not want the office set in the session for performance reasons or does it cause some other error? Is this only for HQ or CWMS or some other special case?

[MikeNeilson]

Do you not want the office set in the session for performance reasons or does it cause some other error? Is this only for HQ or CWMS or some other special case?

Changing the office is what's causing the permissions error. On the national systems the web user has "HQ/CWMS Users" only so if you switch the office for that user (in this context there's no additional session user that would have permission) it fails to even set the office because it doesn't have any permissions for that office.

This wasn't an issue in testing for the following reasons

1. most of us use a single office, so this works.
2. We started adding permissions to the web user for each office in the tests that write data (it is needed for writing data)

I don't see a huge performance difference either way, and the remove of connectionResult was because there was no need to set the session and passing null wouldn't work as that connection user could have more than one office.

I mean, it does save a round trip to the database for everything connection query and that does add up to a lot, but this is focused on more consistent behavior. For reads we've always used the default permissions of the connection user and this was going beyond that.

[MikeNeilson]

I'll need to setup some actually tests to demostrate behavior but I wanted to get some comment on direction first. Option 1 above just means going to those databases and adding /CWMS User to the CDA connection user, the PR is a code change.

[MikeNeilson]

Do you not want the office set in the session for performance reasons or does it cause some other error? Is this only for HQ or CWMS or some other special case?

Oh I don't think it was using the connection. In that attempt I removed getDslContext(connection, office) and repalced it with dsl.configuration(). So it opened a connection, but then tried to open another connection to actually call the function. In my gradle.properties for testing I set max connections to 1 to expose this type of issue, so this was user error.

[rma-rripken]

I've been annoyed by all the setOffice calls. In the back of my mind I'm cooking up a scheme involving an internal connection pool per office. Then it might be possible to get a pre-warmed connection and not have to do the setOffice call everytime. Obviously lots of ways it could go wrong but with some knobs and feedback it seems like it should be possible

[MikeNeilson]

I've been annoyed by all the setOffice calls. In the back of my mind I'm cooking up a scheme involving an internal connection pool per office. Then it might be possible to get a pre-warmed connection and not have to do the setOffice call everytime. Obviously lots of ways it could go wrong but with some knobs and feedback it seems like it should be possible

... Just, no.

[rma-rripken]

Awesome. Thanks for the constructive feedback.

[MikeNeilson]

Awesome. Thanks for the constructive feedback.

Sorry, that cause my brain to panic right before a meeting started and wanted to say something, should said more.

That sounds like a maintanance and setup night. I think I get why you want to do it, but 30 different pools means keeping 30 connections open, likely more with any horizontal scaling.

It also doesn't buy any performance improvement as it's not the office that matters, it's the (office,user pair) that happening for every request. So we'd still need to set the session to the correct user for the database to determine if the operation can be performed. The next level of the rabit hole is a connection per user... but then you don't really have a pool anymore.

There's also whether the authorization bits stay at that level of the database, if they don't, the need to call the session is simply overcome by events and neither the call or the connection pool per office is necessary.

I'm certainly not opposed to multiple pools, we're definitely going to need one for the eventual message queue integration as those are intentionally long running, and a 3rd pool for known and can't help it long running tasks, like loading a 100.00 to 600.00 by .01 ft elavation/capacity table makes sense. But just to avoid a reasonably quick call to the database to establish permissions because we're dealing with a legacy system doesn't seem like it.