TCR-66 mod-rtac-cache initial review by rcorr-loc · Pull Request #120 · folio-org/tech-council

rcorr-loc · 2026-04-23T20:27:21Z

https://folio-org.atlassian.net/browse/TCR-66
https://github.com/folio-org/mod-rtac-cache

Add initial review

CLAassistant · 2026-04-23T20:27:28Z

All committers have signed the CLA.

rcorr-loc · 2026-04-23T20:51:55Z

+  * evidence: https://github.com/folio-org/mod-rtac-cache/blob/master/PERSONAL_DATA_DISCLOSURE.md
+  * RECOMMENDED CHANGE: the module uses circulation/loans, so consider:
+    * prev:
+      * [x] This module does not store any PD.


retain "does not store any PD." - revised section as below

prev:

This module does not store any PD.

This module does not process any PD.

change to:

This module does not store any PD.

This module transmits PD (including queues, additional databases, etc.)

Circulation transaction(s)

Thanks @rcorr-loc . This is a suggested change in PERSONAL_DATA_DISCLOSURE.md file in our module right? Just checking my understanding about the process here. This module consumes circulation events and http responses from circ that can contain patron PII. We don't do anything with this patron info however. But the events/responses do contain it. So saying it "transmits" is true in that sense.

julianladisch · 2026-04-24T07:54:57Z

+  * much of FOLIO hasn't been TC evaluated, but only uses interfaces from core modules
+* [x] Gracefully handles the absence of third party systems or related configuration. (3, 5, 12)
+  * _Note: This applies to optional third-party integrations and their configurations only. Required environment variables (those without sensible defaults) should fail fast on startup per the Environment Variables Policy._
+* [x] Sonarqube hasn't identified any security issues, any high or greater severity issues, or excessive (>3%) duplication (6); and any disabled or intentionally ignored rules/recommendations are reasonably justified.


https://github.com/folio-org/mod-rtac-cache/blob/v1.1.0/pom.xml#L32-L35 lacks justification

@julianladisch I think this has been removed. Can this comment be resolved? Same below.

julianladisch · 2026-04-24T08:17:13Z

+  * much of FOLIO hasn't been TC evaluated, but only uses interfaces from core modules
+* [x] Gracefully handles the absence of third party systems or related configuration. (3, 5, 12)
+  * _Note: This applies to optional third-party integrations and their configurations only. Required environment variables (those without sensible defaults) should fail fast on startup per the Environment Variables Policy._
+* [x] Sonarqube hasn't identified any security issues, any high or greater severity issues, or excessive (>3%) duplication (6); and any disabled or intentionally ignored rules/recommendations are reasonably justified.


https://github.com/folio-org/mod-rtac-cache/blob/v1.1.0/src/main/java/org/folio/rtaccache/repository/RtacHoldingRepositoryImpl.java#L27
Suppression scope is too broad (complete function), should go to line 67 as single line suppression.

Justification should explain why we cannot or don't want to use a method that returns a TypedQuery that doesn't require an unchecked conversion.

@julianladisch I've removed both cases of suppression in the following PR: folio-org/mod-rtac-cache#51

julianladisch · 2026-04-24T08:25:10Z

+      * POST /_/tenant
+      * DELETE /_/tenant
+* [ ] Environment variables comply with [FOLIO Policy](https://folio-org.atlassian.net/wiki/x/NYCgTg) (5, 11)
+  * [ ] [All environment variables documented in ModuleDescriptor with required fields](https://folio-org.atlassian.net/wiki/spaces/TC/pages/1319141429/Environment+Variables#3.3-ModuleDescriptor-Enhancement)


There's no top level "env" entry.
The "launchDescriptor"->"env" entry is a minimal default configuration for test environments and is not intended as documentation.

@julianladisch can you please clarify this comment? We're obviously missing the default and description keys in our env array. Are you saying we should add those?

We do have a env entry in the launchDescriptor. That appears to be where other modules have placed them (not "top level" in other words).

Please see ticket and PR. Added env array at root.

update TCR-66 eval to incorporate sonarqube suppression and sql hotspot changes, top level env in module descriptor

rcorr-loc · 2026-06-03T17:35:50Z

Updated TCR-66 md file to incorporate above changes

Summary of remaining proposed changes (for further discussion on accuracy/need):

RECOMMENDED CHANGE: there are ~20 Category B third party licences identified in prev tech eval tool run -> add to README
RECOMMENDED CHANGE: the module uses circulation/loans, so consider:
- prev:
  - This module does not store any PD.
  - This module does not process any PD.
- change to:
  - This module does not store any PD.
  - This module transmits PD (including queues, additional databases, etc.)
  - Circulation transaction(s)
RECOMMENDED CHANGE: env var naming: MAX_POLL_RECORDS -> KAFKA_MAX_POLL_RECORDS
RECOMMENDED CHANGE: required column was recently added to top level env in module descriptor - question on whether the required column also needs to be added to README.md

Beyond above, I think everything looks good

rcorr-loc · 2026-06-04T15:12:31Z

Updated TCR-66 md file to incorporate above changes

Summary of remaining proposed changes (for further discussion on accuracy/need):

RECOMMENDED CHANGE: there are ~20 Category B third party licences identified in prev tech eval tool run -> add to README
RECOMMENDED CHANGE: the module uses circulation/loans, so consider:
- prev:
  - This module does not store any PD.
  - This module does not process any PD.
- change to:
  - This module does not store any PD.
  - This module transmits PD (including queues, additional databases, etc.)
  - Circulation transaction(s)
RECOMMENDED CHANGE: env var naming: MAX_POLL_RECORDS -> KAFKA_MAX_POLL_RECORDS
RECOMMENDED CHANGE: required column was recently added to top level env in module descriptor - question on whether the required column also needs to be added to README.md

Beyond above, I think everything looks good

steveellis · 2026-06-05T16:29:07Z

* RECOMMENDED CHANGE: required column was recently added to top level env in module descriptor - question on whether the required column also needs to be added to README.md

@rcorr-loc I think you're talking about the DB_PASSWORD env var. This was marked as required in the readme in the PR here.

Regarding the other changes I have created a ticket for them here: https://folio-org.atlassian.net/browse/MODRTACHCE-58.

rcorr-loc · 2026-06-05T16:49:45Z

@steveellis ok thanks for MODRTACHCE-58

the last thing on my list is the third party licenses - what about adding the Category B third party licenses to the readme?
I can try running the evaluator tool (https://github.com/folio-org/tc-module-eval) again this eve, but last I tried it seemed like there were ~20

I don't seem many good examples of other repos adopting this tc req but I think it is more recent @okolawole-ebsco

rcorr-loc · 2026-07-13T15:07:22Z

Updated TCR-66 to note MODRTACHCE-58 changes, which have been merged to main/master

TCR-66 initial review

adb3fe8

Add initial review

rcorr-loc requested a review from a team as a code owner April 23, 2026 20:27

rcorr-loc commented Apr 23, 2026

View reviewed changes

julianladisch reviewed Apr 24, 2026

View reviewed changes

julianladisch changed the title ~~TCR-66 initial review~~ TCR-66 mod-rtac-cache initial review May 13, 2026

Update TCR-66_2026-04-23-mod-rtac-cache.MD

a036bf7

update TCR-66 eval to incorporate sonarqube suppression and sql hotspot changes, top level env in module descriptor

Update TCR-66 post MODRTACHE-58

22f9e45

Uh oh!

Conversation

rcorr-loc commented Apr 23, 2026 • edited by julianladisch Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CLAassistant commented Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rcorr-loc Apr 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rcorr-loc commented Jun 3, 2026

Uh oh!

rcorr-loc commented Jun 4, 2026

Uh oh!

steveellis commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rcorr-loc commented Jun 5, 2026

Uh oh!

rcorr-loc commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rcorr-loc commented Apr 23, 2026 •

edited by julianladisch

Loading

CLAassistant commented Apr 23, 2026 •

edited

Loading

rcorr-loc Apr 23, 2026 •

edited

Loading

steveellis commented Jun 5, 2026 •

edited

Loading

rcorr-loc commented Jul 13, 2026 •

edited

Loading