Skip to content

feat(backend): cache database and graphql api per user; move web -in/out from database to session #5196

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

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

mswertz
Copy link
Member

@mswertz mswertz commented Jul 4, 2025

demo:

  • created a GraphqlSession that is lightweight because it uses a cache of GraphqlApi and Database instances per user/per schema
  • removed Database.setActiveUser so users cannot be switched within database, to prevent accidental errors in that cache with wrong user assigned
  • created runAsUser and runAsAdmin for use cases where database temporarily needs to run in different/elevated privileges

todo:

  • removed Database.setActive user and replaced with runAsUser where it seems appriopriate
  • replace raw use of Database/Graphql with using the GraphqlSession class with caching
  • rework graphql tests to use this GraphqlSession so we can see the caching indeed works
  • beacon needed a lot of changes as it used many of the 'raw' Database and Schema objects
  • seems to be a bug around setSettings
  • see if we can reduce the amount of changes needed (56 files is a bit much)

@mswertz mswertz marked this pull request as draft July 4, 2025 22:45
@mswertz mswertz self-assigned this Jul 9, 2025
@connoratrug connoratrug requested a review from Copilot July 17, 2025 07:13
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces a per-user caching layer for Database and GraphQL API instances and shifts session/user tracking out of raw Database calls into a dedicated GraphqlSession class.

  • Added GraphqlSession and GraphqlSessionFieldFactory to centralize session logic and cache heavy objects per user.
  • Updated SqlDatabase and Database interfaces to replace setActiveUser/becomeAdmin with runAsUser/runAsAdmin.
  • Refactored controllers, API factories, and almost all tests to use GraphqlSession or the new SqlDatabase(String, boolean) constructor.

Reviewed Changes

Copilot reviewed 56 out of 56 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
backend/molgenis-emx2/src/main/java/org/molgenis/emx2/DatabaseListener.java Added databaseChanged flag and getter to track full-DB changes.
backend/molgenis-emx2/src/main/java/org/molgenis/emx2/Database.java / SqlDatabase.java Removed setActiveUser/becomeAdmin, added runAsUser/runAsAdmin.
backend/molgenis-emx2-webapi/src/main/java/org/molgenis/emx2/web/MolgenisSessionManager.java / MolgenisSession.java Switched from manual DB wrappers to MolgenisSession subclass of GraphqlSession.
backend/molgenis-emx2-graphql/src/main/java/org/molgenis/emx2/graphql/GraphqlSession.java New session class with Caffeine caches for DB and GraphQL instances.
backend/molgenis-emx2-graphql/src/main/java/org/molgenis/emx2/graphql/GraphqlApiFactory.java / GraphqlSessionFieldFactory.java Adapted GraphQL factory to use GraphqlSession in field definitions.
Multiple test files Updated imports and instantiation to use SqlDatabase or GraphqlSession.
Comments suppressed due to low confidence (3)

backend/molgenis-emx2/src/main/java/org/molgenis/emx2/DatabaseListener.java:29

  • [nitpick] Rename getDatabaseChanged() to isDatabaseChanged() to follow standard boolean getter naming conventions.
  public boolean getDatabaseChanged() {

backend/molgenis-emx2-graphql/src/main/java/org/molgenis/emx2/graphql/GraphqlSession.java:33

  • Using a static TaskService may lead to shared mutable state across sessions; consider making it instance-based or verifying thread-safety.
  private static TaskService taskService = new TaskServiceInMemory();

@@ -21,7 +21,10 @@ public class MolgenisSessionManager {
private static final Logger logger = LoggerFactory.getLogger(MolgenisSessionManager.class);
private Map<String, MolgenisSession> sessions = new ConcurrentHashMap<>();

public MolgenisSessionManager() {}
public MolgenisSessionManager() {
// set static settigs
Copy link
Preview

Copilot AI Jul 17, 2025

Choose a reason for hiding this comment

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

Correct spelling in the comment: 'settigs' should be 'settings'.

Suggested change
// set static settigs
// set static settings

Copilot uses AI. Check for mistakes.

Comment on lines +28 to +33
Caffeine.newBuilder().maximumSize(1000).expireAfterAccess(5, TimeUnit.MINUTES).build();
private static Cache<String, GraphQL> schemaApiCachePerUser =
Caffeine.newBuilder().maximumSize(1000).expireAfterAccess(5, TimeUnit.MINUTES).build();
private static Cache<String, GraphQL> databaseApiCachePerUser =
Caffeine.newBuilder().maximumSize(1000).expireAfterAccess(5, TimeUnit.MINUTES).build();
private static TaskService taskService = new TaskServiceInMemory();
Copy link
Contributor

Choose a reason for hiding this comment

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

expose settings to (runtime) config

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