RHINENG-23942 - Add support for expiration_date to remediations endpoints in the API backend#1005
Open
RHINENG-23942 - Add support for expiration_date to remediations endpoints in the API backend#1005
Conversation
Contributor
Reviewer's GuideAdds expiration_date as a first-class field on remediations, including DB schema/migration, model, config, seed data, API contract, query filtering/sorting, write semantics, and comprehensive tests around the new behavior. Sequence diagram for create and patch remediations with expiration_datesequenceDiagram
actor User
participant APIBackend
participant ControllerWrite
participant Remediation
participant Config
%% Create remediation with explicit expiration_date
User->>APIBackend: POST /v1/remediations { name, auto_reboot, expiration_date }
APIBackend->>ControllerWrite: create(req, res)
ControllerWrite->>ControllerWrite: parseExpirationDate(bodyExpirationDate)
ControllerWrite->>Remediation: create({ name, auto_reboot, expiration_date, ... })
Remediation-->>ControllerWrite: persisted remediation
ControllerWrite-->>APIBackend: 201 { id }
APIBackend-->>User: response
%% Create remediation without expiration_date (default from retention policy)
User->>APIBackend: POST /v1/remediations { name, auto_reboot }
APIBackend->>ControllerWrite: create(req, res)
ControllerWrite->>Config: read planRetentionDays
Config-->>ControllerWrite: days
ControllerWrite->>ControllerWrite: defaultExpirationDate()
ControllerWrite->>Remediation: create({ name, auto_reboot, expiration_date=now+days, ... })
Remediation-->>ControllerWrite: persisted remediation
ControllerWrite-->>APIBackend: 201 { id }
APIBackend-->>User: response
%% Patch remediation updating expiration_date
User->>APIBackend: PATCH /v1/remediations/:id { expiration_date }
APIBackend->>ControllerWrite: patch(req, res)
ControllerWrite->>Remediation: findByPk(id)
Remediation-->>ControllerWrite: remediation instance
ControllerWrite->>ControllerWrite: parseExpirationDate(bodyExpirationDate)
ControllerWrite->>Remediation: set expiration_date
ControllerWrite->>Remediation: save()
Remediation-->>ControllerWrite: updated remediation
ControllerWrite-->>APIBackend: 200 { id }
APIBackend-->>User: response
ER diagram for remediations table with expiration_dateerDiagram
REMEDIATIONS {
uuid id PK
string name
boolean auto_reboot
boolean needs_reboot
boolean archived
string account_number
string tenant_org_id
string created_by
string updated_by
string workspace_id
date expiration_date
timestamp created_at
timestamp updated_at
}
REMEDIATION_ISSUES {
uuid id PK
uuid remediation_id FK
string issue_id
string resolution
integer precedence
}
REMEDIATIONS ||--o{ REMEDIATION_ISSUES : has
Class diagram for remediations expiration_date supportclassDiagram
class Config {
+number planRetentionDays
+number migrationBackfillBatchSize
}
class DbHelpers {
+string getDefaultExpirationDate()
}
class RemediationModel {
+UUID id
+string name
+boolean auto_reboot
+boolean needs_reboot
+boolean archived
+string account_number
+string tenant_org_id
+string created_by
+string updated_by
+string workspace_id
+Date expiration_date
+Date created_at
+Date updated_at
+Date defaultValueExpirationDate()
}
class RemediationsControllerWrite {
+Date defaultExpirationDate()
+Date parseExpirationDate(value)
+create(req, res)
+patch(req, res)
}
class RemediationsQueries {
+list(tenant_org_id, created_by, system, primaryOrder, asc, filter, detailed, limit, offset)
}
class MigrationAddExpirationDateToRemediations {
+up(q, DataTypes)
+down(q)
}
Config --> RemediationModel : uses planRetentionDays
Config --> RemediationsControllerWrite : uses planRetentionDays
Config --> MigrationAddExpirationDateToRemediations : uses planRetentionDays
Config --> MigrationAddExpirationDateToRemediations : uses migrationBackfillBatchSize
DbHelpers --> Config : uses
DbHelpers --> RemediationsSeeders : used by
RemediationsControllerWrite --> RemediationModel : create/patch instances
RemediationsControllerWrite --> Config : uses planRetentionDays
RemediationsQueries --> RemediationModel : reads list with expiration_date
MigrationAddExpirationDateToRemediations --> RemediationModel : schema change
class RemediationsSeeders {
+up(q)
}
RemediationsSeeders --> DbHelpers : calls getDefaultExpirationDate
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The logic for computing default expiration dates is now duplicated in the migration, model default, controller.write (defaultExpirationDate), and db/helpers.getDefaultExpirationDate; consider centralizing this into a single helper to avoid subtle drift if the behavior needs to change later.
- In the expiration_date backfill migration, the batched UPDATE uses a subselect with LIMIT but no ORDER BY; adding an ORDER BY (e.g., created_at or id) would make the batching deterministic and can help the planner perform more predictable, index-friendly scans.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The logic for computing default expiration dates is now duplicated in the migration, model default, controller.write (defaultExpirationDate), and db/helpers.getDefaultExpirationDate; consider centralizing this into a single helper to avoid subtle drift if the behavior needs to change later.
- In the expiration_date backfill migration, the batched UPDATE uses a subselect with LIMIT but no ORDER BY; adding an ORDER BY (e.g., created_at or id) would make the batching deterministic and can help the planner perform more predictable, index-friendly scans.
## Individual Comments
### Comment 1
<location path="src/remediations/remediations.queries.js" line_range="260-261" />
<code_context>
+ // expiration_date filters (before, after, or expiring within N days)
+ const expConditions = [];
+ if (filter.expiration_before) {
+ expConditions.push({ [Op.lte]: new Date(filter.expiration_before) });
+ }
+ if (filter.expiration_after) {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Date-only filters using `new Date('YYYY-MM-DD')` may behave differently across timezones.
`new Date('2026-12-31')` depends on how the engine interprets date-only strings, which varies by timezone (UTC vs local). That can shift the effective filter boundary versus the DB’s `DATE`/`TIMESTAMP` column. If these are calendar date filters, consider explicitly building a UTC range (start/end of day) instead of relying on `new Date(string)` parsing.
Suggested implementation:
```javascript
const ISSUE_ATTRIBUTES = ['issue_id', 'resolution', 'precedence'];
function parseDateOnlyToUtcStart(dateStr) {
if (typeof dateStr !== 'string') {
return null;
}
const match = /^(\d{4})-(\d{2})-(\d{2})$/.exec(dateStr);
if (!match) {
return null;
}
const year = Number(match[1]);
const month = Number(match[2]) - 1; // JS months are 0-based
const day = Number(match[3]);
return new Date(Date.UTC(year, month, day, 0, 0, 0, 0));
}
function parseDateOnlyToUtcEnd(dateStr) {
if (typeof dateStr !== 'string') {
return null;
}
const match = /^(\d{4})-(\d{2})-(\d{2})$/.exec(dateStr);
if (!match) {
return null;
}
const year = Number(match[1]);
const month = Number(match[2]) - 1; // JS months are 0-based
const day = Number(match[3]);
return new Date(Date.UTC(year, month, day, 23, 59, 59, 999));
}
const PLAYBOOK_RUN_ATTRIBUTES = [
```
```javascript
// expiration_date filters (before, after, or expiring within N days)
const expConditions = [];
if (filter.expiration_before) {
const endOfDayUtc = parseDateOnlyToUtcEnd(filter.expiration_before);
if (endOfDayUtc) {
expConditions.push({ [Op.lte]: endOfDayUtc });
}
}
if (filter.expiration_after) {
const startOfDayUtc = parseDateOnlyToUtcStart(filter.expiration_after);
if (startOfDayUtc) {
expConditions.push({ [Op.gte]: startOfDayUtc });
}
}
```
```javascript
if (filter.expiring_within_days !== undefined && filter.expiring_within_days !== '') {
const n = parseInt(filter.expiring_within_days, 10);
if (!Number.isNaN(n) && n >= 0) {
const now = new Date();
now.setUTCHours(0, 0, 0, 0);
const end = new Date(now);
end.setUTCDate(end.getUTCDate() + n);
expConditions.push({ [Op.gte]: now });
```
</issue_to_address>
### Comment 2
<location path="db/migrations/20251201120000-add-expiration-date-to-remediations.js" line_range="9-10" />
<code_context>
+
+module.exports = {
+ up: async (q, {DATE}) => {
+ const retentionDays = config.planRetentionDays;
+ const batchSize = config.migrationBackfillBatchSize;
+
+ await q.addColumn('remediations', 'expiration_date', {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Relying on application config in a migration can make behavior environment-dependent and harder to reproduce.
Because `planRetentionDays` and `migrationBackfillBatchSize` come from runtime config, rerunning this migration or running it in another environment with different env vars will change the backfilled values and batch sizes. For schema/data migrations, it’s safer to hardcode these values (or at least assert/log the expected ones) so the behavior is fixed and reproducible even if `PLAN_RETENTION_DAYS` changes later.
Suggested implementation:
```javascript
'use strict';
const config = require('../../src/config');
// Fixed values for this migration to keep behavior reproducible even if config/env changes later.
const RETENTION_DAYS = 30; // snapshot of planRetentionDays at time of writing this migration
const BACKFILL_BATCH_SIZE = 1000; // snapshot of migrationBackfillBatchSize at time of writing this migration
// Surface any drift between migration expectations and current runtime config.
if (
config.planRetentionDays !== RETENTION_DAYS ||
config.migrationBackfillBatchSize !== BACKFILL_BATCH_SIZE
) {
// eslint-disable-next-line no-console
console.warn(
`Migration 20251201120000-add-expiration-date-to-remediations is using fixed values ` +
`RETENTION_DAYS=${RETENTION_DAYS}, BACKFILL_BATCH_SIZE=${BACKFILL_BATCH_SIZE}, ` +
`but runtime config is planRetentionDays=${config.planRetentionDays}, ` +
`migrationBackfillBatchSize=${config.migrationBackfillBatchSize}.`
);
}
const EXPIRATION_DATE_INDEX = 'remediations_expiration_date_idx';
```
```javascript
up: async (q, {DATE}) => {
const retentionDays = RETENTION_DAYS;
const batchSize = BACKFILL_BATCH_SIZE;
```
1. Replace the placeholder values `RETENTION_DAYS = 30` and `BACKFILL_BATCH_SIZE = 1000` with the actual values currently used in your `config.planRetentionDays` and `config.migrationBackfillBatchSize` so this migration accurately captures the intended behavior at the time it was written.
2. If your linting rules differ (e.g., no top-level console usage), you may need to adjust or remove the `console.warn` and use your project's preferred logging mechanism instead.
</issue_to_address>
### Comment 3
<location path="db/helpers.js" line_range="11-13" />
<code_context>
+ */
+function getDefaultExpirationDate() {
+ const config = require('../src/config');
+ const d = new Date();
+ d.setDate(d.getDate() + (config.planRetentionDays ?? 270));
+ return d.toISOString().split('T')[0];
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Returning a date string for `expiration_date` while the column is `DATE`/`TIMESTAMP` can introduce subtle type/format inconsistencies.
This helper makes seeders write `YYYY-MM-DD` strings into a `DATE` column, while other flows (model default, controller create/patch) send JS `Date` instances. That inconsistency can lead to dialect-specific casting differences and forces downstream code to handle both strings and Dates. Consider returning a `Date` and letting Sequelize serialize it so the type matches the rest of the code path.
Suggested implementation:
```javascript
```
```javascript
/**
* Returns a default expiration_date for seed data: today + planRetentionDays.
* Computed at seed time so remediation plans stay "not yet expired" whenever
* tests run. Future culling jobs can test with explicitly past expiration_date
* in dedicated seed data or fixtures.
*
* Returns a Date instance so Sequelize can serialize it consistently with
* other code paths (model defaults, controllers, etc).
*/
function getDefaultExpirationDate() {
const config = require('../src/config');
const d = new Date();
d.setDate(d.getDate() + (config.planRetentionDays ?? 270));
return d;
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+260
to
+261
| if (filter.expiration_before) { | ||
| expConditions.push({ [Op.lte]: new Date(filter.expiration_before) }); |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Date-only filters using new Date('YYYY-MM-DD') may behave differently across timezones.
new Date('2026-12-31') depends on how the engine interprets date-only strings, which varies by timezone (UTC vs local). That can shift the effective filter boundary versus the DB’s DATE/TIMESTAMP column. If these are calendar date filters, consider explicitly building a UTC range (start/end of day) instead of relying on new Date(string) parsing.
Suggested implementation:
const ISSUE_ATTRIBUTES = ['issue_id', 'resolution', 'precedence'];
function parseDateOnlyToUtcStart(dateStr) {
if (typeof dateStr !== 'string') {
return null;
}
const match = /^(\d{4})-(\d{2})-(\d{2})$/.exec(dateStr);
if (!match) {
return null;
}
const year = Number(match[1]);
const month = Number(match[2]) - 1; // JS months are 0-based
const day = Number(match[3]);
return new Date(Date.UTC(year, month, day, 0, 0, 0, 0));
}
function parseDateOnlyToUtcEnd(dateStr) {
if (typeof dateStr !== 'string') {
return null;
}
const match = /^(\d{4})-(\d{2})-(\d{2})$/.exec(dateStr);
if (!match) {
return null;
}
const year = Number(match[1]);
const month = Number(match[2]) - 1; // JS months are 0-based
const day = Number(match[3]);
return new Date(Date.UTC(year, month, day, 23, 59, 59, 999));
}
const PLAYBOOK_RUN_ATTRIBUTES = [ // expiration_date filters (before, after, or expiring within N days)
const expConditions = [];
if (filter.expiration_before) {
const endOfDayUtc = parseDateOnlyToUtcEnd(filter.expiration_before);
if (endOfDayUtc) {
expConditions.push({ [Op.lte]: endOfDayUtc });
}
}
if (filter.expiration_after) {
const startOfDayUtc = parseDateOnlyToUtcStart(filter.expiration_after);
if (startOfDayUtc) {
expConditions.push({ [Op.gte]: startOfDayUtc });
}
} if (filter.expiring_within_days !== undefined && filter.expiring_within_days !== '') {
const n = parseInt(filter.expiring_within_days, 10);
if (!Number.isNaN(n) && n >= 0) {
const now = new Date();
now.setUTCHours(0, 0, 0, 0);
const end = new Date(now);
end.setUTCDate(end.getUTCDate() + n);
expConditions.push({ [Op.gte]: now });
Comment on lines
+11
to
+13
| const d = new Date(); | ||
| d.setDate(d.getDate() + (config.planRetentionDays ?? 270)); | ||
| return d.toISOString().split('T')[0]; |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): Returning a date string for expiration_date while the column is DATE/TIMESTAMP can introduce subtle type/format inconsistencies.
This helper makes seeders write YYYY-MM-DD strings into a DATE column, while other flows (model default, controller create/patch) send JS Date instances. That inconsistency can lead to dialect-specific casting differences and forces downstream code to handle both strings and Dates. Consider returning a Date and letting Sequelize serialize it so the type matches the rest of the code path.
Suggested implementation:
/**
* Returns a default expiration_date for seed data: today + planRetentionDays.
* Computed at seed time so remediation plans stay "not yet expired" whenever
* tests run. Future culling jobs can test with explicitly past expiration_date
* in dedicated seed data or fixtures.
*
* Returns a Date instance so Sequelize can serialize it consistently with
* other code paths (model defaults, controllers, etc).
*/
function getDefaultExpirationDate() {
const config = require('../src/config');
const d = new Date();
d.setDate(d.getDate() + (config.planRetentionDays ?? 270));
return d;
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add expiration_date support to remediation plans, including database schema, API contracts, and defaulting behavior, and update configuration, seed data, and tests accordingly.
New Features:
Enhancements:
Build:
Tests: