Skip to content

Only wrap ehr_lookups tables whose pseudo-PK differs from the real PK #1168

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.

Sign up for GitHub

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
labkey-martyp merged 2 commits into from
Jul 23, 2026
Merged

Only wrap ehr_lookups tables whose pseudo-PK differs from the real PK #1168

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
212d269
Only wrap ehr_lookups tables whose pseudo-PK differs from the real PK
labkey-martyp Jul 22, 2026
f0a6b71
Read the real PK from the wrapped table instead of re-querying DbSchema
labkey-martyp Jul 23, 2026
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
14 changes: 11 additions & 3 deletions ehr/src/org/labkey/ehr/query/EHRLookupsUserSchema.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,10 +212,18 @@ else if (EHRSchema.TABLE_LOOKUP_SETS.equalsIgnoreCase(name))

// By default, any hard tables in the ehr_lookups schema not accounted for above will fall into one of the
// two categories below. Both of these will add a check that makes sure the user has EHRDataAdminPermission
// in order to insert/update/delete on the table. The ContainerScopedTable case is for those tables that
// have a true DB PK or rowid but a User pseudoPK that should be accounted for at the container level.
// in order to insert/update/delete on the table. The ContainerScopedTable case is for tables whose
// user-facing key (promoted via isKeyField in ehr_lookups.xml) differs from the true DB PK: the PK
// constraint does not enforce uniqueness of the pseudo-PK, so the wrapper enforces it per container and
// resolves the pseudo-PK to the real PK on update. Tables whose user-facing key is the true PK
// (e.g. project_types) get CustomPermissionsTable instead: the PK constraint already enforces uniqueness,
// and container-scoping such a table made its key column non-insertable in the UI.
String pkColName = getPkColName(ti);
if (pkColName != null && !"rowid".equalsIgnoreCase(pkColName) && ti.getColumn("container") != null)
List<String> realPk = _dbSchema.getTable(name).getPkColumnNames();

@bbimber bbimber Jul 23, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

My only question is whether this adds overhead. I suspect that LK might do enough TableInfo caching that this doesnt matter. Second question is whether the TableInfo could be interrogated directly (since it probably stores a reference to the schema table info), rather than calling _dbSchema.getTable(), which would ensure we dont add the overhead of re-querying the DbSchema for that TableInfo.

If you did your own investigation and dont think these issues are worth it, I dont have that strong of an opinion here.

@labkey-martyp labkey-martyp Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, good point on getting the realPk from the existing ti realTable. That change is pushed.

And to your first point, there's nothing here that will re-query the tableinfo metadata. That will already be in the SchemaTableInfoCache. So should be negligible impact on performance.

boolean singleColumnPks = pkColName != null && realPk.size() == 1;
boolean realPkMatchesPseudoPk = singleColumnPks && realPk.get(0).equalsIgnoreCase(pkColName);

if (singleColumnPks && !realPkMatchesPseudoPk && ti.getColumn("container") != null)
return getContainerScopedTable(name, cf, pkColName, EHRDataAdminPermission.class);
else
return getCustomPermissionTable(createSourceTable(name), cf, EHRDataAdminPermission.class);
Expand Down