Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,8 @@ project/plugins/project/
*.ear

# virtual machine crash logs, see http://www.java.com/en/download/help/error_hotspot.xml
core
core.[0-9]*
hs_err_pid*

### OSX ###
Expand Down
26 changes: 18 additions & 8 deletions docs/ai/CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,17 +34,27 @@ instantiated via `jdbi.onDemand()`.

## ConsentModule singleton pattern

Every `@Provides` method in `ConsentModule` that creates a new service or DAO instance uses
`@Singleton` + `synchronized` + a lazy null-guard field to guarantee a single instance
on both the Guice injection path and the direct inter-provider call path:
Every `@Provides` method in `ConsentModule` that creates a new service or DAO instance is
annotated `@Singleton`, and takes each of its dependencies as a method parameter so Guice
resolves them. Guice caches the singleton itself, so no lazy field or `synchronized` guard
is needed:

```java
@Provides
@Singleton
synchronized EmailService providesEmailService() {
if (emailService == null) {
emailService = new EmailService(...);
}
return emailService;
private DatasetService providesDatasetService(
Jdbi jdbi,
DatasetServiceDAO datasetServiceDAO,
ElasticSearchService elasticSearchService,
EmailService emailService,
OntologyService ontologyService) {
return new DatasetService(
jdbi, datasetServiceDAO, elasticSearchService, emailService, ontologyService);
}
```

Never call one `@Provides` method from another. A direct call bypasses Guice's scoping and
builds a second instance with its own `jdbi.onDemand` DAOs. Declare the dependency as a
parameter instead. Adding a new service means adding a provider here — a service that is
only JIT-bound (constructed by Guice without a declared provider) is unscoped, so a second
injection point silently creates a second instance.
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@
import org.broadinstitute.consent.http.resources.SamResource;
import org.broadinstitute.consent.http.resources.SigningOfficialDashboardResource;
import org.broadinstitute.consent.http.resources.StatusResource;
import org.broadinstitute.consent.http.resources.StudyAssetResource;
import org.broadinstitute.consent.http.resources.StudyCommentResource;
import org.broadinstitute.consent.http.resources.StudyDatasetTemplateResource;
import org.broadinstitute.consent.http.resources.StudyResource;
import org.broadinstitute.consent.http.resources.SupportResource;
Expand Down Expand Up @@ -200,6 +202,8 @@ public void run(ConsentConfiguration config, Environment env) {
env.jersey().register(injector.getInstance(StatusResource.class));
env.jersey().register(injector.getInstance(StudyDatasetTemplateResource.class));
env.jersey().register(injector.getInstance(StudyResource.class));
env.jersey().register(injector.getInstance(StudyAssetResource.class));
env.jersey().register(injector.getInstance(StudyCommentResource.class));
env.jersey().register(injector.getInstance(SupportResource.class));
env.jersey().register(injector.getInstance(TDRResource.class));
env.jersey().register(injector.getInstance(TosResource.class));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@
import org.broadinstitute.consent.http.service.OntologyService;
import org.broadinstitute.consent.http.service.ResearcherDashboardService;
import org.broadinstitute.consent.http.service.SigningOfficialDashboardService;
import org.broadinstitute.consent.http.service.StudyAssetService;
import org.broadinstitute.consent.http.service.StudyCommentService;
import org.broadinstitute.consent.http.service.SupportRequestService;
import org.broadinstitute.consent.http.service.UseRestrictionConverter;
import org.broadinstitute.consent.http.service.UserService;
Expand Down Expand Up @@ -635,8 +637,21 @@ private CounterService providesCounterService(Jdbi jdbi) {

@Provides
@Singleton
private MetricsService providesMetricsService(Jdbi jdbi) {
return new MetricsService(jdbi);
private MetricsService providesMetricsService(Jdbi jdbi, DatasetService datasetService) {
return new MetricsService(jdbi, datasetService);
}

@Provides
@Singleton
private StudyAssetService providesStudyAssetService(DatasetService datasetService) {
return new StudyAssetService(datasetService);
}

@Provides
@Singleton
private StudyCommentService providesStudyCommentService(
Jdbi jdbi, DatasetService datasetService) {
return new StudyCommentService(jdbi, datasetService);
}

@Provides
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,12 @@ protected boolean authorize(AuthUser authUser, String role) {
boolean authorize = false;
try {
User user = userService.findUserByEmail(authUser.getEmail());
// A user with no user_role rows has a null role list, not an empty one. Without this guard
// the authorizer throws a NullPointerException and Jersey turns what should be a plain
// denial into a 500 on every @RolesAllowed endpoint. Mirrors User#hasAnyUserRole.
if (user == null || user.getRoles() == null) {
return false;
}
Comment thread
otchet-broad marked this conversation as resolved.
return user.getRoles().stream().anyMatch(r -> r.getName().equalsIgnoreCase(role));
} catch (NotFoundException e) {
logWarn("User not found, authorization incomplete: %s".formatted(authUser.getEmail()));
Expand Down
Loading
Loading