Bugfix/apikey header capitalization #447
Conversation
… for the setClientInfo call.
…url. Also reduced logging for normal conditions at info.
MikeNeilson
added
hec-approved-W912HQ22F0913-13b
| private static String connectionUser = null; | ||
| private static String defaultOffice = null; |
There was a problem hiding this comment.
Making these static and conditionally setting them in the constructor weirds me out. I can't say exactly why but it feels like asking for trouble. Like if the first request made is for an un-checked get end-point and the request includes office_id will it cause the static value to get assigned to whatever the first request provided? At the very minimum it seems like assigning to AuthDao.defaultOffice shouldn't happen if the set_session_user_direct failed - right?
There was a problem hiding this comment.
This always uses the office id of the context "e.g. cwms -> hq, spk -> spk" not the context of data.
For the 2nd part I think it just won't get used, but I don't think the additional logic is required just for that state.
There was a problem hiding this comment.
But the short version is that I'm just trying to avoid creating a new instance of this object on every request since it will be used on a LOT of requests.
There was a problem hiding this comment.
ok. the part about the context makes more sense. Should AuthDao have a static getInstance() and just use it singleton style?
There was a problem hiding this comment.
I considered the use of a singleton, that's going to need more investigation. Since the clientInfo is different for every request i needs the new context on each run so I think for now we do new to just run new on every request since their could be multiple threads. Or synchronize on the object which seems like a terrible idea.
There was a problem hiding this comment.
Okay, after realizing it needs the javalin Context information of the current request so that setClientInfo is correct for tracing instead of a pure singleton I made it a singleton per thread.
| } | ||
| AuthDao.RETRIEVE_GROUPS_OF_USER = ResourceHelper.getResourceAsString("/cwms/data/sql/user_groups.sql",this.getClass()); |
There was a problem hiding this comment.
Is there a reason to do this here vs at the variable declaration?
There was a problem hiding this comment.
I'm quite sure there was when I initial did the work.
There was a problem hiding this comment.
Hmm, that's not a bad idea. Let me chew on that one a bit. It is supposed to be usable by all the possible Auth mechanisms.
There was a problem hiding this comment.
Well, that response was to the comment above but I did go ahead and just do this.
…-threaded environment.
User haven't been:
the new setClientInfo on the JDBC connections would end up too long and fail.