Allow inserts on ContainerScopedTable when pseudo-key is the real PK

[labkey-martyp]

Rationale

ContainerScopedTable.init() hid and disabled every real PK column, assuming the real PK is always a surrogate key (e.g. an auto-incrementing rowid). When the pseudo-key is also the real PK (a natural key, like ehr_lookups.project_types.type), this made the key column hidden and non-editable, so it could not be entered through the single-row insert UI (API-based inserts were unaffected).

Related Pull Requests

None.

Changes

• Detect when the pseudo-key column is itself the real PK and leave it visible and editable so it can be entered in the single-row insert UI; other real PK columns are hidden and disabled as before.
• Editing the column remains unavailable in the UI: it is hidden from the update form via setShownInUpdateView(false), so it is enterable on insert but not shown when editing.

[bbimber]

@labkey-martyp and @labkey-bpatel: I dont think there is necessarily a problem with this; however, if the true PK is the pseudo PK, why is the upstream code using ContainerScopedTable? Is there anything that ContainerScopedTable actually adds?

I see that there are examples in EHR where a table is using ContainerScopedTable, where it probably should just use CustomPermissionsTable, such as EHRUserSchema line 79. I think these two would be more appropriately set as CustomPermissionsTable, although I doubt it would have created a problem until something like this (https://github.com/LabKey/ehrModules/blob/a53bc60337522fd1b10757b18aaa045fa5b6e828/ehr/src/org/labkey/ehr/query/EHRUserSchema.java#L80)

        else if (EHRSchema.TABLE_PROTOCOL.equalsIgnoreCase(name))
            return getCustomPermissionTable(createSourceTable(name), cf, EHRProtocolEditPermission.class);
        else if (EHRSchema.TABLE_PROJECT.equalsIgnoreCase(name))
            return getCustomPermissionTable(createSourceTable(name), cf, EHRProjectEditPermission.class);

Since this mentions project_type, I assume the source of that is here: https://github.com/LabKey/ehrModules/blob/a53bc60337522fd1b10757b18aaa045fa5b6e828/ehr/src/org/labkey/ehr/query/EHRLookupsUserSchema.java#L219. I cant think of any reason why that logic is right. Again, outside of editing a PK, it probably never mattered either. I bet this was introduced in ~2018 when container was added to a lot of these tables (one commit of many: LabKey/ehrModules@03e8338)

This code in EHRLookupUserSchema should be more like this: https://github.com/LabKey/ehrModules/blob/a53bc60337522fd1b10757b18aaa045fa5b6e828/ehr/src/org/labkey/ehr/query/EHRLookupsUserSchema.java#L217

            // 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.
            // This only applies if there is a column named rowid
            String pkColName = getPkColName(ti);
            if (pkColName != null && ti.getColumn("rowid") != null && "rowid".equalsIgnoreCase(pkColName) &&  ti.getColumn("container") != null)
                return getContainerScopedTable(name, cf, pkColName, EHRDataAdminPermission.class);
            else
                return getCustomPermissionTable(createSourceTable(name), cf, EHRDataAdminPermission.class);

[labkey-martyp]

@bbimber I see your point in the EHRLookupsUserSchema. I would actually propose that fallback option never uses ContainerScopedTable. It just passes the natural key as the pseudo-key and ContainerScopedTable adds no value in that configuration. The db will just throw an error for duplicates regardless of container. I'll open a PR there instead.

I do think ehr.project/protocol need to be ContainerScopedTables as their pk is objectid, so they need something to ensure unique project/protocols per container.

[bbimber]

@bbimber I see your point in the EHRLookupsUserSchema. I would actually propose that fallback option never uses ContainerScopedTable. It just passes the natural key as the pseudo-key and ContainerScopedTable adds no value in that configuration. The db will just throw an error for duplicates regardless of container. I'll open a PR there instead.

I do think ehr.project/protocol need to be ContainerScopedTables as their pk is objectid, so they need something to ensure unique project/protocols per container.

Hi @labkey-martyp: I did not look at Protocol/Project very closely, so if there is a different PK then I agree we should not touch them. That code in EHRLookupsUserSchema is the source of the protocol_types issue pointed out above.

To that point on 'Fallback option', if you mean line 221, which uses CustomPermissionsTable not ContainerScopedTable, then I think the existing code is right. In summary, I think you just need to change the logic on line line 217 to test if "rowid" exists on the table. That one change matches the intention of what the original code was probably trying to do.

Since ContainerScopedTable doesnt behave properly when the true PK and pseudoPK are identical, I'd suggest adding error or warning logging to ContainerScopedTable that this is a developer/configuration error, as a standalone PR. We could let TC tell us whether it's happening elsewhere. I would not add this PR, since I'd argue silently changing the display on the true PK is masking a developer/config error.

[labkey-martyp]

@bbimber Oh ok, you swapped the boolean on "rowid".equalsIgnoreCase(pkColName) in your suggestion for EHRLookupsUserSchema. That's why I wasn't understanding what you're suggesting. That fix makes sense with !"rowid".equalsIgnoreCase(pkColName).

[bbimber]

@bbimber Oh ok, you swapped the boolean on "rowid".equalsIgnoreCase(pkColName) in your suggestion for EHRLookupsUserSchema. That's why I wasn't understanding what you're suggesting. That fix makes sense with !"rowid".equalsIgnoreCase(pkColName).

Sorry - I typed this into github. Should have read it better. Sounds like we agree here.

[labkey-martyp]

Per comments above. Tackling this particular issue in EHRLookupsUserSchema.