Skip to content
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

[ALS-7694] Add DashboardDrawer feature with controller, service, and repository #55

Merged
merged 7 commits into from
Nov 20, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ public List<Map<String, String>> getHackyBDCRows() {
String sql =
"""
SELECT
dataset.dataset_id as dataset_id,
Copy link
Author

Choose a reason for hiding this comment

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

Can use the dataset table's primary key the return object to make downstream processes more efficient

dataset.abbreviation AS abbreviation,
dataset.full_name AS name,
CASE
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package edu.harvard.dbmi.avillach.dictionary.dashboarddrawer;

import java.util.List;

public record DashboardDrawer(
int datasetId, String studyFullname, String studyAbbreviation, List<String> consentGroups, String studySummary, List<String> studyFocus,
String studyDesign, String sponsor
) {
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
package edu.harvard.dbmi.avillach.dictionary.dashboarddrawer;
Copy link
Member

Choose a reason for hiding this comment

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

What's the difference between edu.harvard.dbmi.avillach.dictionary.dashboarddrawer and edu.harvard.dbmi.avillach.dictionary.dashboard as far as business purpose goes? Are these legitimately different domains, or is this a BDC specific implementation of the same domain?

Copy link
Author

Choose a reason for hiding this comment

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

Yea I was in the same boat at the beginning and was trying to fit it into to the Dashboard thus why it was basically a replication of it until changed the class names.

I think I should follow up on this ticket with Cleaning up some of the dashboard method and looking at how this integrates with the dashboard more closely.

From more of a backend mindset I think it should relate to a dataset endpoint and not a feature endpoint.


import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.http.ResponseEntity;
import org.springframework.stereotype.Controller;
import org.springframework.web.bind.annotation.*;

@Controller
@RequestMapping("/dashboard-drawer")
public class DashboardDrawerController {

@Autowired
private DashboardDrawerService dashboardDrawerService;

@GetMapping
public ResponseEntity<DashboardDrawerList> findAll() {
return ResponseEntity.ok(dashboardDrawerService.findAll());
}

@GetMapping("/{id}")
public ResponseEntity<DashboardDrawer> findByDatasetId(@PathVariable Integer id) {
return ResponseEntity.ok(dashboardDrawerService.findByDatasetId(id));
Copy link
Member

Choose a reason for hiding this comment

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

This should 404 if the dataset DNE. You can do this cleanly by expressing the nullable DashboardDrawer as an Optional<DashboardDrawer> and then mapping that to a response entity here.

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
package edu.harvard.dbmi.avillach.dictionary.dashboarddrawer;

import java.util.List;

public record DashboardDrawerList(List<DashboardDrawer> dashboardDrawerList) {
Copy link
Member

Choose a reason for hiding this comment

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

You made yourself a note to get rid of this record and just return a list. I agree with that note.

Copy link
Author

Choose a reason for hiding this comment

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

I made a comment that DashboardDrawerRecord was redundant as it's already a record.

DashboardDrawerList should be a list of DashboardDrawer as that is the collection type of this record class.

DashboardDrawer is just a record.

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
package edu.harvard.dbmi.avillach.dictionary.dashboarddrawer;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.jdbc.core.namedparam.MapSqlParameterSource;
import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate;
import org.springframework.stereotype.Repository;

import java.util.*;

@Repository
public class DashboardDrawerRepository {

private final NamedParameterJdbcTemplate template;

private static final Logger log = LoggerFactory.getLogger(DashboardDrawerRepository.class);

@Autowired
public DashboardDrawerRepository(NamedParameterJdbcTemplate template) {
this.template = template;
}

public List<DashboardDrawer> getDashboardDrawerRows() {
String materializedViewSql = """
Copy link
Author

Choose a reason for hiding this comment

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

Materialized views are pretty powerful tools. I am not in love with how Postgres handles them as you have to build logic to use them unlike Oracles Query Rewrite abilities.

https://docs.oracle.com/cd/B10501_01/server.920/a96520/qr.htm

This query is not expensive so I am not worried about needing this unless we scale number of studies and retrieved metadata. Can remove materialized view logic if needed.

Would make the query much more dynamic / possibly stored procedure. Aggregation gets expensive.

Copy link
Member

Choose a reason for hiding this comment

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

Let's talk about this in OH. I clearly don't use views often enough and I want to learn more about them from you.

select * from dictionary_db.dict.dataset_meta_materialized_view dmmv;
""";

String fallbackSql = """
SELECT d.dataset_id,
MAX(d.full_name) study_fullname,
MAX(d.abbreviation) study_abbreviation,
ARRAY_AGG(DISTINCT c.description) consent_groups,
MAX(d.description) study_summary,
ARRAY_AGG(DISTINCT dm.value) FILTER (where dm.key IN ('study_focus')) study_focus,
MAX(DISTINCT dm.value) FILTER (where dm.key IN ('study_design')) study_design,
MAX(DISTINCT dm.value) FILTER (where dm.key IN ('sponsor')) sponsor
FROM dataset d
JOIN dataset_meta dm ON d.dataset_id = dm.dataset_id
JOIN consent c ON d.dataset_id = c.dataset_id
GROUP BY d.dataset_id
""";

try {
return template.query(materializedViewSql, new DashboardDrawerRowMapper());
} catch (Exception e) {
log.debug("Materialized view not available, using fallback query. Error: {}", e.getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

Why can't we make it so this view always exists?

Copy link
Author

Choose a reason for hiding this comment

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

https://www.postgresql.org/docs/current/rules-materializedviews.html

Materialized views are a bit different then views. As in they are stored on disk ( in memory on some DBMS ). They are basically a point in time view of a table. But they need to be managed and refreshed.

Have not use Postgres in my checkered past but they were a Saint when it came to Oracle. Oracle has query rewrite which makes it much easier to manage. From what I see so far in Postgres we will need to write logic blocks like this or queries will fail if Materialized view is being rewritten.

Query rewrite would fall back on the select statement used to create the materialized view if the view doesn't exist or is being refreshed.

return template.query(fallbackSql, new DashboardDrawerRowMapper());
}
}

public List<DashboardDrawer> getDashboardDrawerRows(Integer datasetId) {
Copy link
Author

Choose a reason for hiding this comment

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

Same as other query but this will find a specific record based on the datasetId

Copy link
Member

Choose a reason for hiding this comment

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

This is selecting a specific dataset ID. Why return a list when it's always either 1 or 0 elements?

String materializedViewSql = """
select * from dictionary_db.dict.dataset_meta_materialized_view dmmv where dmmv.dataset_id = :datasetId;
""";

String fallbackSql = """
SELECT d.dataset_id dataset_id,
MAX(d.full_name) study_fullname,
MAX(d.abbreviation) study_abbreviation,
ARRAY_AGG(DISTINCT c.description) consent_groups,
MAX(d.description) study_summary,
ARRAY_AGG(DISTINCT dm.value) FILTER (where dm.key IN ('study_focus')) study_focus,
MAX(DISTINCT dm.value) FILTER (where dm.key IN ('study_design')) study_design,
MAX(DISTINCT dm.value) FILTER (where dm.key IN ('sponsor')) sponsor
FROM dataset d
JOIN dataset_meta dm ON d.dataset_id = dm.dataset_id
JOIN consent c ON d.dataset_id = c.dataset_id
where d.dataset_id = :datasetId
GROUP BY d.dataset_id
""";
MapSqlParameterSource params = new MapSqlParameterSource();
params.addValue("datasetId", datasetId);

try {
return template.query(materializedViewSql, params, new DashboardDrawerRowMapper());
} catch (Exception e) {
log.debug("Materialized view not available, using fallback query. Error: {}", e.getMessage());
return template.query(fallbackSql, params, new DashboardDrawerRowMapper());
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package edu.harvard.dbmi.avillach.dictionary.dashboarddrawer;

import org.springframework.jdbc.core.RowMapper;

import java.sql.ResultSet;
import java.sql.SQLException;
import java.sql.Array; // For handling SQL Array
import java.util.Arrays;
import java.util.List;

public class DashboardDrawerRowMapper implements RowMapper<DashboardDrawer> {

Copy link
Author

Choose a reason for hiding this comment

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

I would at least make an abstract class for this that I could extend to fulfill multiple Dashboard Layouts.

Should probably do this for other Classes such as Dashboard.

@Override
public DashboardDrawer mapRow(ResultSet rs, int rowNum) throws SQLException {
return new DashboardDrawer(
rs.getInt("dataset_id"), rs.getString("study_fullname"), rs.getString("study_abbreviation"),
convertSqlArrayToList(rs.getArray("consent_groups")), rs.getString("study_summary"),
convertSqlArrayToList(rs.getArray("study_focus")), rs.getString("study_design"), rs.getString("sponsor")
);
}

private List<String> convertSqlArrayToList(Array sqlArray) throws SQLException {
if (sqlArray == null) {
return List.of();
} else {
Object[] arrayContents = (Object[]) sqlArray.getArray();
// Check if the array contains a single empty value
if (arrayContents.length == 1 && "".equals(arrayContents[0])) {
Copy link
Author

Choose a reason for hiding this comment

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

This is a bit hacky but sqlArray returns an single empty value for empty values in the db. Which makes sense as empty values can have meaning. For this component they do not.

return List.of();
} else {
return Arrays.asList((String[]) sqlArray.getArray());
}
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
package edu.harvard.dbmi.avillach.dictionary.dashboarddrawer;

import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.stereotype.Service;

import java.util.ArrayList;
import java.util.List;

@Service
public class DashboardDrawerService {

private final DashboardDrawerRepository repository;
private final String dashboardLayout;

@Autowired
public DashboardDrawerService(DashboardDrawerRepository repository, @Value("${dashboard.layout.type}") String dashboardLayout) {
this.repository = repository;
this.dashboardLayout = dashboardLayout;
}

/**
* Retrieves the Dashboard Drawer for all datasets.
*
* @return a Dashboard instance with drawer-specific columns and rows.
*/
public DashboardDrawerList findAll() {
if (dashboardLayout.equalsIgnoreCase("bdc")) {
List<DashboardDrawer> records = repository.getDashboardDrawerRows();
return new DashboardDrawerList(records);
}

return new DashboardDrawerList(new ArrayList<>());
}

/**
* Retrieves the Dashboard Drawer for a specific dataset.
*
* @param datasetId the ID of the dataset to fetch.
* @return a Dashboard instance with drawer-specific columns and rows.
*/
public DashboardDrawer findByDatasetId(Integer datasetId) {
if (dashboardLayout.equalsIgnoreCase("bdc")) {
List<DashboardDrawer> records = repository.getDashboardDrawerRows(datasetId);
// Should be atomic as the query is an aggregation on the dataset table.
// Probably a better way to do this.
if (records.size() == 1) {
return records.getFirst();
}
}

return new DashboardDrawer(-1, "", "", new ArrayList<>(), "", new ArrayList<>(), "", "");
Copy link
Member

Choose a reason for hiding this comment

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

I don't like this zero value, and I want to understand more about the API requirements justify this.

}
}
5 changes: 4 additions & 1 deletion src/main/resources/application-bdc.properties
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,8 @@ spring.datasource.username=${DATASOURCE_USERNAME}
server.port=80

dashboard.enable.extra_details=true
dashboard.enable.bdc_hack=true
dashboard.layout.type=bdc
Luke-Sikina marked this conversation as resolved.
Show resolved Hide resolved

filtering.unfilterable_concepts=stigmatized

filtering.unfilterable_concepts=stigmatized
4 changes: 3 additions & 1 deletion src/main/resources/application.properties
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,14 @@ spring.datasource.url=jdbc:postgresql://${POSTGRES_HOST}:5432/${POSTGRES_DB}?cur
spring.datasource.username=${POSTGRES_USER}
spring.datasource.password=${POSTGRES_PASSWORD}
spring.datasource.driver-class-name=org.postgresql.Driver

server.port=80

dashboard.columns={abbreviation:'Abbreviation',name:'Name',clinvars:'Clinical Variables'}
dashboard.column-order=abbreviation,name,clinvars
dashboard.nonmeta-columns=abbreviation,name
dashboard.enable.extra_details=true
dashboard.enable.bdc_hack=true
dashboard.enable.bdc_hack=false
dashboard.layout.type=default

filtering.unfilterable_concepts=stigmatized
2 changes: 1 addition & 1 deletion src/test/resources/application.properties
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ spring.datasource.driver-class-name=org.postgresql.Driver
dashboard.columns={abbreviation:'Abbreviation',melast:'This one goes last',name:'Name',clinvars:'Clinical Variables',participants:'Participants'}
dashboard.column-order=abbreviation,name,clinvars
dashboard.nonmeta-columns=abbreviation,name

dashboard.enable.extra_details=true
dashboard.enable.bdc_hack=false
dashboard.layout.type=default

filtering.unfilterable_concepts=stigmatized