Skip to content

DT-1662: Ensure single Library Card per User #2536

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

Merged
merged 28 commits into from
May 28, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
74865a0
wip: first pass at making LCs singular - compilation and most tests pass
rushtong May 22, 2025
fd1b491
revert
rushtong May 22, 2025
b4a7a3a
unused import
rushtong May 22, 2025
5a5009d
reduce log level
rushtong May 22, 2025
ad471eb
formatting
rushtong May 22, 2025
ffa0e2c
test fix
rushtong May 22, 2025
18b1dac
test fix
rushtong May 22, 2025
cbf0a45
test fix
rushtong May 22, 2025
c3dce93
test fix
rushtong May 23, 2025
0d3a35e
test fix
rushtong May 23, 2025
e62bc4e
enforce in db
rushtong May 23, 2025
31b496f
swagger model update
rushtong May 23, 2025
e1b102c
populate both for temporary compatibility
rushtong May 23, 2025
0dbb51e
fix test for unique violation
rushtong May 23, 2025
5271514
new tests for updated user queries
rushtong May 23, 2025
5490d01
retain comment for helper method
rushtong May 23, 2025
098ee34
rm query only used in test
rushtong May 23, 2025
c1f2a61
delete duplicates
rushtong May 23, 2025
0cecb3b
fixx test bug
rushtong May 23, 2025
e8f293b
test updates for unique constraints
rushtong May 23, 2025
0dcf0e5
Merge branch 'develop' into gr-DT-1662-single-lc-per-user
rushtong May 23, 2025
e0a9ba5
test cleanup
rushtong May 23, 2025
a03a0f9
method rename; rm duplicate test
rushtong May 23, 2025
89c17c5
restrict duplicate email rows to null user ids
rushtong May 23, 2025
beacde0
further restrict duplicate user id deletes to non-null user id rows
rushtong May 23, 2025
6ae6055
copilot feedback
rushtong May 23, 2025
a6f4815
copilot feedback - update test for clarity
rushtong May 23, 2025
d6cfedc
Merge branch 'develop' into gr-DT-1662-single-lc-per-user
rushtong May 23, 2025
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
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ WITH daa_deletes AS (DELETE FROM lc_daa lc_daa WHERE lc_daa.lc_id = :libraryCard
LEFT JOIN lc_daa ld ON lc.id = ld.lc_id
WHERE lc.user_id = :userId
""")
List<LibraryCard> findLibraryCardsByUserId(@Bind("userId") Integer userId);
LibraryCard findLibraryCardByUserId(@Bind("userId") Integer userId);

@RegisterBeanMapper(value = LibraryCard.class)
@UseRowReducer(LibraryCardReducer.class)
Expand All @@ -115,7 +115,7 @@ WITH daa_deletes AS (DELETE FROM lc_daa lc_daa WHERE lc_daa.lc_id = :libraryCard
@UseRowReducer(LibraryCardReducer.class)
@SqlQuery("SELECT * FROM library_card " +
"WHERE user_email = :email")
List<LibraryCard> findAllLibraryCardsByUserEmail(@Bind("email") String email);
LibraryCard findLibraryCardByUserEmail(@Bind("email") String email);

@SqlUpdate("DELETE FROM library_card WHERE user_id = :userId OR create_user_id = :userId OR update_user_id = :userId")
void deleteAllLibraryCardsByUser(@Bind("userId") Integer userId);
Expand Down
94 changes: 62 additions & 32 deletions src/main/java/org/broadinstitute/consent/http/db/UserDAO.java
Original file line number Diff line number Diff line change
Expand Up @@ -28,16 +28,38 @@ public interface UserDAO extends Transactional<UserDAO> {
@RegisterBeanMapper(value = User.class, prefix = "u")
@RegisterBeanMapper(value = UserRole.class)
@RegisterBeanMapper(value = Institution.class, prefix = "i")
@RegisterBeanMapper(value = LibraryCard.class, prefix = "lc")
@UseRowReducer(UserWithRolesReducer.class)
@SqlQuery("SELECT "
+ User.QUERY_FIELDS_WITH_U_PREFIX + QUERY_FIELD_SEPARATOR
+ Institution.QUERY_FIELDS_WITH_I_PREFIX + QUERY_FIELD_SEPARATOR
+ " ur.user_role_id, ur.user_id, ur.role_id, ur.dac_id, r.name "
+ " FROM users u "
+ " LEFT JOIN user_role ur ON ur.user_id = u.user_id "
+ " LEFT JOIN roles r ON r.role_id = ur.role_id "
+ " LEFT JOIN institution i ON u.institution_id = i.institution_id"
+ " WHERE u.user_id = :userId")
@SqlQuery("""
SELECT
u.user_id as u_user_id,
u.email as u_email,
u.display_name as u_display_name,
u.create_date as u_create_date,
u.email_preference as u_email_preference,
u.institution_id as u_institution_id,
u.era_commons_id as u_era_commons_id,
i.institution_id as i_id,
i.institution_name as i_name,
i.it_director_name as i_it_director_name,
i.it_director_email as i_it_director_email,
i.create_date as i_create_date,
i.update_date as i_update_date,
ur.user_role_id as ur_user_role_id, ur.user_id as ur_user_id,
ur.role_id as ur_role_id, ur.dac_id as ur_dac_id, r.name as ur_name,
lc.id AS lc_id, lc.user_id AS lc_user_id,
lc.user_name AS lc_user_name, lc.user_email AS lc_user_email,
lc.create_user_id AS lc_create_user_id, lc.create_date AS lc_create_date,
lc.update_user_id AS lc_update_user_id,
ld.daa_id as lc_daa_id
FROM users u
LEFT JOIN user_role ur ON ur.user_id = u.user_id
LEFT JOIN roles r ON r.role_id = ur.role_id
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we pull this common section into something separate referenced by this method and the other one?

Copy link
Contributor

Choose a reason for hiding this comment

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

@fboulnois if we do that, won't we get complaints from SemGrep? Or are you suggesting we also change that configuration at this time?

Copy link
Contributor

Choose a reason for hiding this comment

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

If I had to guess, it's why this class is being refactored in this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I had to guess, it's why this class is being refactored in this way.

This is the primary reason why this is refactored, yes. The string concatenation approach has been problematic to maintain and triggers semgrep issues. I would like to see if we can come up with a better way of re-using large query blocks in some other way, but for now, this is the least bad option as I see it.

LEFT JOIN institution i ON u.institution_id = i.institution_id
LEFT JOIN library_card lc ON lc.user_id = u.user_id
LEFT JOIN lc_daa ld ON lc.id = ld.lc_id
WHERE u.user_id = :userId
""")
User findUserById(@Bind("userId") Integer userId);

@RegisterBeanMapper(value = User.class, prefix = "u")
Expand Down Expand Up @@ -93,17 +115,38 @@ public interface UserDAO extends Transactional<UserDAO> {
@RegisterBeanMapper(value = User.class, prefix = "u")
@RegisterBeanMapper(value = UserRole.class, prefix = "ur")
@RegisterBeanMapper(value = Institution.class, prefix = "i")
@RegisterBeanMapper(value = LibraryCard.class, prefix = "lc")
@UseRowReducer(UserWithRolesReducer.class)
@SqlQuery("SELECT "
+ User.QUERY_FIELDS_WITH_U_PREFIX + QUERY_FIELD_SEPARATOR
+ Institution.QUERY_FIELDS_WITH_I_PREFIX + QUERY_FIELD_SEPARATOR
+ " ur.user_role_id as ur_user_role_id, ur.user_id as ur_user_id, "
+ " ur.role_id as ur_role_id, ur.dac_id as ur_dac_id, r.name as ur_name "
+ " FROM users u "
+ " LEFT JOIN user_role ur ON ur.user_id = u.user_id "
+ " LEFT JOIN roles r ON r.role_id = ur.role_id "
+ " LEFT JOIN institution i ON u.institution_id = i.institution_id"
+ " WHERE LOWER(u.email) = LOWER(:email)")
@SqlQuery("""
SELECT
u.user_id as u_user_id,
u.email as u_email,
u.display_name as u_display_name,
u.create_date as u_create_date,
u.email_preference as u_email_preference,
u.institution_id as u_institution_id,
u.era_commons_id as u_era_commons_id,
i.institution_id as i_id,
i.institution_name as i_name,
i.it_director_name as i_it_director_name,
i.it_director_email as i_it_director_email,
i.create_date as i_create_date,
i.update_date as i_update_date,
ur.user_role_id as ur_user_role_id, ur.user_id as ur_user_id,
ur.role_id as ur_role_id, ur.dac_id as ur_dac_id, r.name as ur_name,
lc.id AS lc_id, lc.user_id AS lc_user_id,
lc.user_name AS lc_user_name, lc.user_email AS lc_user_email,
lc.create_user_id AS lc_create_user_id, lc.create_date AS lc_create_date,
lc.update_user_id AS lc_update_user_id,
ld.daa_id as lc_daa_id
FROM users u
LEFT JOIN user_role ur ON ur.user_id = u.user_id
LEFT JOIN roles r ON r.role_id = ur.role_id
LEFT JOIN institution i ON u.institution_id = i.institution_id
LEFT JOIN library_card lc ON lc.user_id = u.user_id
LEFT JOIN lc_daa ld ON lc.id = ld.lc_id
WHERE LOWER(u.email) = LOWER(:email)
""")
User findUserByEmail(@Bind("email") String email);

@RegisterBeanMapper(value = User.class, prefix = "u")
Expand Down Expand Up @@ -176,19 +219,6 @@ Integer insertUser(@Bind("email") String email,
List<User> describeUsersByRoleAndEmailPreference(@Bind("roleName") String roleName,
@Bind("emailPreference") Boolean emailPreference);

@RegisterBeanMapper(value = User.class, prefix = "u")
@RegisterBeanMapper(value = UserRole.class)
@UseRowReducer(UserWithRolesReducer.class)
@SqlQuery("SELECT "
+ User.QUERY_FIELDS_WITH_U_PREFIX + QUERY_FIELD_SEPARATOR
+ " ur.user_role_id, ur.user_id, ur.role_id, ur.dac_id, r.name "
+ " FROM users u "
+ " LEFT JOIN user_role ur ON ur.user_id = u.user_id "
+ " LEFT JOIN roles r ON r.role_id = ur.role_id "
+ " WHERE LOWER(u.email) = LOWER(:email) "
+ " AND r.role_id = :roleId")
User findUserByEmailAndRoleId(@Bind("email") String email, @Bind("roleId") Integer roleId);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only used in tests


@UseRowMapper(UserWithRolesMapper.class)
@SqlQuery("select du.*, r.role_id, r.name, ur.user_role_id, ur.user_id, ur.role_id, ur.dac_id " +
" from users du " +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ public void accumulate(Map<Integer, DarCollection> map, RowView rowView) {
user.setInstitution(institution);
}
if (Objects.nonNull(libraryCard)) {
user.addLibraryCard(libraryCard);
user.setLibraryCard(libraryCard);
}
if (Objects.nonNull(userProperty)) {
user.addProperty(userProperty);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

import java.util.Map;
import java.util.Objects;
import java.util.Optional;
import org.broadinstitute.consent.http.models.Institution;
import org.broadinstitute.consent.http.models.LibraryCard;
import org.broadinstitute.consent.http.models.User;
Expand Down Expand Up @@ -33,19 +32,12 @@ public void accumulate(Map<Integer, User> map, RowView rowView) {
id -> rowView.getRow(User.class));

try {
// Some queries look for `user_role_id` while those that use a prefix look for `u_user_role_id`
Integer userRoleId = null;
if (hasNonZeroColumn(rowView, "user_role_id")) {
userRoleId = rowView.getColumn("user_role_id", Integer.class);
} else if (hasNonZeroColumn(rowView, "ur_user_role_id")) {
userRoleId = rowView.getColumn("ur_user_role_id", Integer.class);
}
if (Objects.nonNull(userRoleId)) {
UserRole ur = rowView.getRow(UserRole.class);
user.addRole(ur);
UserRole userRole = mapUserRoleFromRowView(rowView, userId);
if (userRole != null) {
user.addRole(userRole);
}
} catch (MappingException e) {
// Ignore any attempt to map a column that doesn't exist
logWarn("Error adding User Role to User", e);
}
try {
if (Objects.nonNull(rowView.getColumn("i_id", Integer.class))) {
Expand All @@ -56,34 +48,56 @@ public void accumulate(Map<Integer, User> map, RowView rowView) {
}
}
} catch (MappingException e) {
//Ignore institution mapping errors, possible for new users to not have an institution
logDebug("Error adding Institution to User: %s".formatted(e.getMessage()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding the case of the debug logs. I find them helpful for local development.

}
//user role join can cause duplication of data if done in tandem with joins on other tables
//ex) The same LC can end up being repeated multiple times
//Below only adds LC if not currently saved on the array
try {
if (rowView.getColumn("lc_id", Integer.class) != null) {
int lcId = rowView.getColumn("lc_id", Integer.class);
LibraryCard lc;
Optional<LibraryCard> existingLibraryCard = user.getLibraryCards().stream()
.filter(card -> card.getId().equals(lcId))
.findFirst();
lc = existingLibraryCard.orElseGet(() -> rowView.getRow(LibraryCard.class));
LibraryCard lc = rowView.getRow(LibraryCard.class);
if (rowView.getColumn("lc_daa_id", Integer.class) != null) {
lc.addDaa(rowView.getColumn("lc_daa_id", Integer.class));
}
user.addLibraryCard(lc);
user.setLibraryCard(lc);
}
} catch (MappingException e) {
//Ignore exceptions here, user may not have a library card issued under this instiution
logDebug("Error adding Library Card to User: %s".formatted(e.getMessage()));
}
try {
if (Objects.nonNull(rowView.getColumn("up_property_id", Integer.class))) {
UserProperty p = rowView.getRow(UserProperty.class);
user.addProperty(p);
}
} catch (MappingException e) {
// Ignore any attempt to map a column that doesn't exist
logDebug("Error adding User Property to User: %s".formatted(e.getMessage()));
}
}

// Some queries look for `user_role_id` while those that use a prefix look for `u_user_role_id`
private UserRole mapUserRoleFromRowView(RowView rowView, Integer userId) {
Comment on lines +77 to +78
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory, I don't think this method should be necessary. However, I found that the mapping wasn't working for cases where the prefix was ur_ and couldn't get that to work like I think it should.

Integer userRoleId;
Integer roleId;
String name;
Integer dacId = null;
// Some queries look for `user_role_id` while those that use a prefix look for `ur_user_role_id`
if (hasNonZeroColumn(rowView, "user_role_id")) {
userRoleId = rowView.getColumn("user_role_id", Integer.class);
roleId = rowView.getColumn("role_id", Integer.class);
name = rowView.getColumn("name", String.class);
if (hasNonZeroColumn(rowView, "dac_id")) {
dacId = rowView.getColumn("dac_id", Integer.class);
}
return new UserRole(userRoleId, userId, roleId, name, dacId);
} else if (hasNonZeroColumn(rowView, "ur_user_role_id")) {
userRoleId = rowView.getColumn("ur_user_role_id", Integer.class);
roleId = rowView.getColumn("ur_role_id", Integer.class);
name = rowView.getColumn("ur_name", String.class);
if (hasNonZeroColumn(rowView, "ur_dac_id")) {
dacId = rowView.getColumn("ur_dac_id", Integer.class);
}
return new UserRole(userRoleId, userId, roleId, name, dacId);
}
return null;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import java.util.ArrayList;
import java.util.Date;
import java.util.List;
import java.util.Objects;
import org.apache.commons.lang3.builder.EqualsBuilder;

public class LibraryCard {
Expand Down Expand Up @@ -116,6 +117,12 @@ public boolean equals(Object libraryCard) {
return new EqualsBuilder().append(id, other.getId()).isEquals();
}

@Override
public int hashCode() {
return Objects.hash(id, userId, userName, userEmail, createDate, createUserId, updateDate,
updateUserId, daaIds);
}

public void addDaa(Integer daaId) {
if (this.daaIds == null) {
this.daaIds = new ArrayList<>();
Expand Down
24 changes: 6 additions & 18 deletions src/main/java/org/broadinstitute/consent/http/models/User.java
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public class User {

private Institution institution;

private List<LibraryCard> libraryCards;
private LibraryCard libraryCard;

public User() {
}
Expand Down Expand Up @@ -101,7 +101,7 @@ public User(String json) {
// Nor do we need to retrieve the full institution object from user-provided data.
JsonObject filteredUserJsonObject = filterFields(
userJsonObject,
Arrays.asList("createDate", "institution", "libraryCards"));
Arrays.asList("createDate", "institution", "libraryCard"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we also have to support libraryCards until they're completely deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we do. This method constructs a User object. Since User doesn't have a libraryCards field so if a user-provided string does contain libraryCards, it will be ignored on the next line in gson.fromJson()

User u = gson.fromJson(filteredUserJsonObject.toString(), User.class);
setUserId(u);
setEmail(u);
Expand Down Expand Up @@ -278,15 +278,12 @@ public Institution getInstitution() {
return institution;
}

public void setLibraryCards(List<LibraryCard> cards) {
this.libraryCards = cards;
public LibraryCard getLibraryCard() {
return libraryCard;
}

public List<LibraryCard> getLibraryCards() {
if (this.libraryCards == null) {
this.libraryCards = new ArrayList<>();
}
return this.libraryCards;
public void setLibraryCard(LibraryCard libraryCard) {
this.libraryCard = libraryCard;
}

public void addRole(UserRole userRole) {
Expand All @@ -308,15 +305,6 @@ public void addProperty(UserProperty userProp) {
}
}

public void addLibraryCard(LibraryCard card) {
if (this.libraryCards == null) {
this.libraryCards = new ArrayList<>();
}
if (!this.libraryCards.contains(card)) {
this.libraryCards.add(card);
}
}

@Override
public int hashCode() {
return this.getUserId();
Expand Down
Loading
Loading