use second db handle for only for user admin and writes #1184

melange396 · 2023-05-26T00:10:32Z

change 'user_engine' to a 'WriteSession' instead, so the master db connection is used for writes only

…nnection is used for writes only

…tag engines with id and log those too, and superfluous user method cleanup

melange396 · 2023-06-13T20:48:23Z

cherry-picked and force updated to set these changes against the current dev branch

…ng for engine creation

…ove session ctx mgrs to admin page

melange396 · 2023-06-20T21:16:55Z

updated change summary:

changed user_engine to WriteSession.
made sure sql statements and timing are logged for all engines, and engines are tagged with identifiers.
added eager-loading of roles when fetching users from db via orm.
removed some unnecessary methods -- those used very few times, or some made irrelevant by the eager-loaded roles.
added @default_session provider decorator.
- allows a default (and hopefully appropriate) session for the decorated method, which will be used as a context manager if an already open session is not passed instead.
removed individual session context managers from individual User ORM method calls, and replaced them with the @default_session decorator.
- i tried to add the 'session' argument to @default_session decorated functions as the last non-defaulted argument, so we didnt need to put 'session=None', but it looks kinda weird and isnt a very standardizing convention. suggestions for alternatives are welcome.
added session context managers to the admin page implementation, so one session can be used for a whole (single) admin operation.

…newly updated User

dmytrotsko

Nice work!

dmytrotsko · 2023-06-21T11:21:46Z

src/server/admin/models.py

@@ -25,7 +25,7 @@ def _default_date_now():
 class User(Base):
    __tablename__ = "api_user"
    id = Column(Integer, primary_key=True, autoincrement=True)
-    roles = relationship("UserRole", secondary=association_table)
+    roles = relationship("UserRole", secondary=association_table, lazy="joined") # last arg does an eager load of this property from foreign tables


Wow, that is awesome. Didn't know about that param.

krivard

Some questions about TODOs and when to/not-to use the decorators, but no dealbreakers. Beautiful work!

src/server/_common.py

src/server/_db.py

src/server/admin/models.py

krivard · 2023-06-21T18:46:13Z

src/server/admin/models.py

+        session.commit()
+        # retrieve the newly updated User object
+        return session.query(User).filter(User.id == user.id).first()


~~is this still needed if we're using WriteSession for both writes and reads in admin?~~

oh i see; this is adding back the commit removed from 99, and lets you return directly instead of needing a separate return at 100

carry on

src/server/admin/models.py

krivard · 2023-06-21T18:52:30Z

src/server/admin/models.py

+            )
+            session.execute(update_stmt)
+            return User._assign_roles(user, roles, session)
+        # TODO: else: raise Exception() ??


does this TODO need to be resolved before merge, or should we file a ticket for it?

¯\_(ツ)_/¯ im ok with neither because our usage of this seems consistent enough right now. a future caller of this can get the same information by checking for a None return value instead of catching an exception. but its easy enough to do, so ill just do it. change incoming shortly...

src/server/admin/models.py

krivard · 2023-06-21T18:54:19Z

src/server/admin/models.py

-            session.commit()
+        """)
+        session.commit()
+        # TODO: look up and return new role


does this TODO need to be resolved before merge, or should we file a ticket for it?

¯\_(ツ)_/¯ im ok with neither, esp because create_role() isnt even used anywhere at the moment. but its easy enough to do, so ill just do it. change incoming shortly...

src/server/endpoints/admin.py

TODOs done: raise Exception when trying to update non-existent User, return UserRole on creation. also use more appropriate reciever for static method call, and expand comment on static vs bound methods in User.

sonarcloud · 2023-06-23T18:55:32Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

No Coverage information
0.0% Duplication

krivard

👍 🚀

melange396 requested a review from dmytrotsko May 26, 2023 00:10

dmytrotsko previously approved these changes May 31, 2023

View reviewed changes

melange396 dismissed dmytrotsko’s stale review via 64a798c June 13, 2023 20:33

melange396 added 2 commits June 13, 2023 16:44

change 'user_engine' to a 'WriteSession' instead, so the master db co…

d36e29a

…nnection is used for writes only

make sure sql statements and timing are logged for all engines, plus …

03c27f2

…tag engines with id and log those too, and superfluous user method cleanup

melange396 force-pushed the master_db_cnxn_write_only branch from 64a798c to 03c27f2 Compare June 13, 2023 20:46

melange396 added 2 commits June 14, 2023 17:10

sqlalchemy cleanup: removed superfluous bits, improved argument passi…

11bafa2

…ng for engine creation

eager-load roles, remove unnecessary methods, add @default_session, m…

2822d9d

…ove session ctx mgrs to admin page

melange396 marked this pull request as ready for review June 20, 2023 21:16

_assign_roles() does its own commit() and returns an instance of the …

3bdd09f

…newly updated User

dmytrotsko previously approved these changes Jun 21, 2023

View reviewed changes

krivard self-requested a review June 21, 2023 14:05

melange396 changed the title ~~[WIP] use second db handle for writes only~~ use second db handle for writes only Jun 21, 2023

melange396 changed the title ~~use second db handle for writes only~~ use second db handle for only for user admin and writes Jun 21, 2023

krivard previously approved these changes Jun 21, 2023

View reviewed changes

address TODOs pointed out in review ++

1c926e7

TODOs done: raise Exception when trying to update non-existent User, return UserRole on creation. also use more appropriate reciever for static method call, and expand comment on static vs bound methods in User.

melange396 dismissed stale reviews from krivard and dmytrotsko via 1c926e7 June 23, 2023 18:53

krivard approved these changes Jun 26, 2023

View reviewed changes

melange396 merged commit 6fe6e7a into dev Jun 26, 2023

melange396 deleted the master_db_cnxn_write_only branch June 26, 2023 15:56

nolangormley mentioned this pull request Jun 27, 2023

Release Delphi Epidata 4.1.4 #1216

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use second db handle for only for user admin and writes #1184

use second db handle for only for user admin and writes #1184

melange396 commented May 26, 2023

melange396 commented Jun 13, 2023

melange396 commented Jun 20, 2023

dmytrotsko left a comment

dmytrotsko Jun 21, 2023

melange396 Jun 23, 2023

krivard left a comment

krivard Jun 21, 2023

melange396 Jun 23, 2023

krivard Jun 21, 2023

melange396 Jun 23, 2023

krivard Jun 21, 2023

melange396 Jun 23, 2023

sonarcloud bot commented Jun 23, 2023

krivard left a comment

use second db handle for only for user admin and writes #1184

use second db handle for only for user admin and writes #1184

Conversation

melange396 commented May 26, 2023

melange396 commented Jun 13, 2023

melange396 commented Jun 20, 2023

dmytrotsko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krivard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Jun 23, 2023

krivard left a comment

Choose a reason for hiding this comment