feat(mocks): record getdata and viewlibs, and show what the mock gets wrong #491

Merged

allan merged 4 commits from feat/parity-getdata-viewlibs into release/7.23 2026-11-07 01:09:32 +00:00

Conversation 7 Commits 4 Files Changed 17

+1904 -45

hermes commented 2026-11-06 21:28:58 +00:00

Collaborator

Copy link

Copy source

Two more scenarios beside public/startupservice, recorded from the seeded control library on the synthetic estate - and the mock brought up to what they show.

What the recorder gained

editors/getdata takes a table input, not URL parameters, so record.js posts one. Two things about that upload are not guessable from the client code: the server reads the file's first line as the SAS input statement (LIBDS :$32. FILTER_RK :best32.), not as a header of column names, and it needs CRLF. Send a plain CSV and the control table arrives with zero observations, which surfaces as syscc=1012 prior to dc_getsettings - a syntax error three steps from the cause. --libref fills a {{LIBREF}} placeholder so the estate's library name stays out of scenarios.json; mockInput gives the mock its own rows at replay, since the two sides do not share a libref.

The harness's webout() stub also grew: it ignored the showmeta option and only wrote the body at CLOSE, which made the mocks' own requireMemberMetadata() guard throw - and meant a service that stopped sending formats and labels would have passed. Both behaviours are asserted in test/harness-webout.test.js.

--scenario accepts service/id as well as an id, because two screens legitimately call their first service default.

The mock, brought up to the recordings

• cols: no DESC key (SAS does not emit one), and LABEL/LONGDESC come from the MPE_DATADICTIONARY rows for the lib.ds.column, as getdata.sas augments mp_getcols; COLTYPE is blank for the temporal columns.
• sasdata: sorted by the business key, character values trimmed as the JSON writer writes them, an all-blank value as a single space - and the current-rows clause that FILTER_TEXT announces is now actually applied, so the grid does not show expired rows while saying they were excluded.
• dqrules: the active MPE_VALIDATIONS rows unioned with the schema's NOTNULL (limited to the columns left in the selection, which is why a NOT NULL on tx_from never reached the grid) and the derived columns' READONLY, de-duplicated and sorted the way getdata.sas does.
• approvers: computed from MPE_SECURITY's APPROVE rows with the admin group as the fallback, instead of a hardcoded user.
• viewlibs: the shape %mp_getlibs answers - engine in LIBRARYID, blank ENGINE, libref as LIBRARYNAME - rather than a nicer one invented by the mock.
• viewdata: a cell the row does not carry is SAS's missing (space for a character column, . for a number) rather than the string undefined, which is what the viewer was rendering.

The control library and the admin group come from services/settings.js through dcInit(), the mock's %mpe_init(). test/mock-settings.test.js proves it by renaming the library and moving the data. The other 13 services still carry const dcLibref = 'DC_JSLIB' and build paths from it at file scope, so converting them is one service at a time.

Status

All three scenarios are gated, with one declared difference each:

ok   public/startupservice/default (8 accepted difference(s))
ok   editors/getdata/mpe_tables (1 accepted difference(s))
ok   public/viewlibs/default (1 accepted difference(s))

getdata: .maxvarlengths[2].MAXLEN - mp_getmaxvarlengths sizes the table's only numeric by the bytes it occupies (3), the mock measures its text (1); the value sizes a grid column. viewlibs: the SASHELP row, which SAS lists and the mock estate has no equivalent of.

dsmeta is excluded from the getdata projection on purpose: it is the engine's report on the dataset (owner, host, encoding, paths, created dates), and the publish check refused the response until it was dropped - the estate's site id and its service account name are in it.

The sas9 mock keeps its own getdata/viewlibs variant and is untouched; the goldens come from the SASjs Server estate.

Verification

node --test sas/mocks/test/*.test.js - 95 pass, 0 fail, including the golden scan, the new settings test, getdata-rules.test.js (an uppercase-keyed APPROVE grant and an open-ended rule, both of which fail on the previous code) and viewdata.test.js.

Two more scenarios beside `public/startupservice`, recorded from the seeded control library on the synthetic estate - and the mock brought up to what they show. ## What the recorder gained `editors/getdata` takes a **table input**, not URL parameters, so `record.js` posts one. Two things about that upload are not guessable from the client code: the server reads the file's **first line as the SAS `input` statement** (`LIBDS :$32. FILTER_RK :best32.`), not as a header of column names, and it needs CRLF. Send a plain CSV and the control table arrives with zero observations, which surfaces as `syscc=1012 prior to dc_getsettings` - a syntax error three steps from the cause. `--libref` fills a `{{LIBREF}}` placeholder so the estate's library name stays out of `scenarios.json`; `mockInput` gives the mock its own rows at replay, since the two sides do not share a libref. The harness's `webout()` stub also grew: it ignored the `showmeta` option and only wrote the body at CLOSE, which made the mocks' own `requireMemberMetadata()` guard throw - and meant a service that stopped sending formats and labels would have passed. Both behaviours are asserted in `test/harness-webout.test.js`. `--scenario` accepts `service/id` as well as an id, because two screens legitimately call their first service `default`. ## The mock, brought up to the recordings - `cols`: no `DESC` key (SAS does not emit one), and `LABEL`/`LONGDESC` come from the `MPE_DATADICTIONARY` rows for the lib.ds.column, as `getdata.sas` augments `mp_getcols`; `COLTYPE` is blank for the temporal columns. - `sasdata`: sorted by the business key, character values trimmed as the JSON writer writes them, an all-blank value as a single space - and the current-rows clause that `FILTER_TEXT` announces is now actually applied, so the grid does not show expired rows while saying they were excluded. - `dqrules`: the active `MPE_VALIDATIONS` rows unioned with the schema's `NOTNULL` (limited to the columns left in the selection, which is why a `NOT NULL` on `tx_from` never reached the grid) and the derived columns' `READONLY`, de-duplicated and sorted the way `getdata.sas` does. - `approvers`: computed from `MPE_SECURITY`'s `APPROVE` rows with the admin group as the fallback, instead of a hardcoded user. - `viewlibs`: the shape `%mp_getlibs` answers - engine in `LIBRARYID`, blank `ENGINE`, libref as `LIBRARYNAME` - rather than a nicer one invented by the mock. - `viewdata`: a cell the row does not carry is SAS's missing (space for a character column, `.` for a number) rather than the string `undefined`, which is what the viewer was rendering. The control library and the admin group come from `services/settings.js` through `dcInit()`, the mock's `%mpe_init()`. `test/mock-settings.test.js` proves it by renaming the library and moving the data. The other 13 services still carry `const dcLibref = 'DC_JSLIB'` and build paths from it at file scope, so converting them is one service at a time. ## Status All three scenarios are gated, with one declared difference each: ``` ok public/startupservice/default (8 accepted difference(s)) ok editors/getdata/mpe_tables (1 accepted difference(s)) ok public/viewlibs/default (1 accepted difference(s)) ``` `getdata`: `.maxvarlengths[2].MAXLEN` - `mp_getmaxvarlengths` sizes the table's only numeric by the bytes it occupies (3), the mock measures its text (1); the value sizes a grid column. `viewlibs`: the `SASHELP` row, which SAS lists and the mock estate has no equivalent of. `dsmeta` is excluded from the getdata projection on purpose: it is the engine's report on the dataset (owner, host, encoding, paths, created dates), and the publish check refused the response until it was dropped - the estate's site id and its service account name are in it. The sas9 mock keeps its own getdata/viewlibs variant and is untouched; the goldens come from the SASjs Server estate. ## Verification `node --test sas/mocks/test/*.test.js` - 95 pass, 0 fail, including the golden scan, the new settings test, `getdata-rules.test.js` (an uppercase-keyed `APPROVE` grant and an open-ended rule, both of which fail on the previous code) and `viewdata.test.js`.

hermes added 1 commit 2026-11-06 21:28:58 +00:00

feat(mocks): record getdata and viewlibs, and show what the mock gets wrong ... 49a9bbf022

Two more scenarios, and the recorder could not take either of them as it stood.

- editors/getdata reads a table input, not URL parameters, so record.js can now
  post one: it writes the informational header the server expects (the first
  line is the SAS input statement, not a list of column names), posts it as
  multipart/form-data, and takes --libref so the estate's control library name
  stays out of scenarios.json. replay.js hands the mock its own rows through
  mockInput, since the two sides do not share a libref.
- the test harness's webout() stub now honours the showmeta option and writes
  the body as members are added. Without them the mocks' own
  requireMemberMetadata() guard throws - it reads the body before CLOSE - and a
  service that stopped sending formats and labels would have passed here.
- --scenario accepts service/id, because two screens legitimately call their
  first service 'default'.

The two goldens are recorded but marked gated: false: the gate prints their
difference counts and does not fail, and check.js labels them ADVISORY. They are
a work list, not an acceptance.

editors/getdata/mpe_tables - 388 differences, all of them the mock's:
cols carries a DESC key SAS never emits and misses the LABEL/LONGDESC/COLTYPE
SAS reads from MPE_DATADICTIONARY; sasdata leaves blank character values out
instead of padding them and is not ordered by the business key; dqrules differ
in content and order; approvers is hardcoded to one user where SAS computes it
and returns none.

public/viewlibs - 4 differences: SAS answers LIBRARYID='V9' with a blank ENGINE
and LIBRARYNAME equal to the libref, and lists SASHELP; the mock answers
LIBRARYID=<libref>, ENGINE='V9', LIBRARYNAME='Data Controller(<libref>)' and has
no SASHELP.

dsmeta is excluded on purpose: it is the engine's report on the dataset (owner,
host, encoding, paths, created dates), estate state rather than behaviour, and
the projection refused the recorded response until it was dropped - the site id
and the service account name were in it.

hermes added 1 commit 2026-11-07 00:31:17 +00:00

fix(mocks): make getdata and viewlibs answer what the estate answers ... 3bb9fb52e3

The two scenarios recorded in the previous commit are now gated, with one
declared difference each instead of 388 and 4. The diffs were the mock's, and
they are fixed rather than accepted:

- cols: dropped the DESC key SAS never emits, and took LABEL/LONGDESC from the
  MPE_DATADICTIONARY rows for this lib.ds.column, as getdata.sas augments
  mp_getcols. COLTYPE is blank for DATE/DATETIME/TIME, which is what the estate
  answers.
- sasdata: sorted by the business key; character values trimmed the way the
  JSON writer writes them, with an all-blank value as a single space.
- dqrules: the active MPE_VALIDATIONS rows unioned with the schema's NOTNULL
  (limited to the columns left in the selection, which is why a NOT NULL on
  tx_from never reached the grid) and the derived columns' READONLY, then
  de-duplicated and sorted by column and rule type with X left at the missing
  value - getdata.sas's union/sort/by-group logic.
- sasparams: the current-rows clause mpe_filtermaster appends, and blank
  character values as the single space SAS sends.
- maxvarlengths: the value as stored (trimmed), and the key columns in upper
  case, which is how the selection spells them.
- approvers: computed from MPE_SECURITY's APPROVE rows with the admin group as
  the fallback, instead of a hardcoded sasdemo.
- viewlibs: the shape %mp_getlibs answers - engine in LIBRARYID, blank ENGINE,
  libref as LIBRARYNAME.

The control library and the admin group now come from services/settings.js
through dcInit(), which is the mock's %mpe_init(): makedata already writes
dc_libref, dc_admin_group and dcpath there, so a deployment with a different
DCPATH no longer needs a service change. test/mock-settings.test.js proves it by
renaming the library and moving the data. The remaining services still carry
const dcLibref = 'DC_JSLIB' and compute paths from it at file scope, so each is
a separate change.

The sas9 mock keeps its own getdata/viewlibs variant, so it is untouched: the
goldens are recorded from the SASjs Server estate.

hermes commented 2026-11-07 00:31:29 +00:00

Author

Collaborator

Copy link

Copy source

Both scenarios are gated now, at 3bb9fb52 - the diffs were the mock's and they are fixed, not accepted. check.js:

ok   public/startupservice/default (8 accepted difference(s))
ok   editors/getdata/mpe_tables (1 accepted difference(s))
ok   public/viewlibs/default (1 accepted difference(s))

getdata, from 388 differences to 1. cols dropped the DESC key and now takes LABEL/LONGDESC from the MPE_DATADICTIONARY rows for this lib.ds.column, the way getdata.sas augments mp_getcols (DSN is "Dataset Name to be edited" again); COLTYPE is blank for the temporal columns. sasdata is sorted by the business key, with character values trimmed as the JSON writer writes them and an all-blank value as a single space. dqrules is the active MPE_VALIDATIONS set unioned with the schema's NOTNULL - limited to the columns left in the selection, which is why a NOT NULL on tx_from never reaches the grid - and the derived columns' READONLY, then de-duplicated and sorted by column and rule type, with X at the missing value. sasparams gained the current-rows clause mpe_filtermaster appends and the space-padded blanks. approvers is computed from MPE_SECURITY's APPROVE rows with the admin group as fallback instead of a hardcoded sasdemo.

The one remaining difference is .maxvarlengths[2].MAXLEN: mp_getmaxvarlengths sizes the table's only numeric by the bytes it occupies (3), the mock measures its text (1). MAXLEN sizes a grid column.

viewlibs, from 4 to 1. The mock now answers the shape %mp_getlibs does - engine in LIBRARYID, blank ENGINE, libref as LIBRARYNAME - rather than a nicer one, because a mock that improves on the payload is a mock the Cypress suite cannot be trusted against. The remaining difference is the SASHELP row, which SAS lists and the mock estate has no equivalent of.

The settings file you mentioned is already there and now has a name for the reader. makedata.js writes services/settings.js with dc_libref, dc_admin_group, dc_staging_area and dcpath - the JS twin of the settings.sas the backend generates in %mpe_init - and dcMockUtils had a loader. I added dcInit() as the one entry point (the mock's %mpe_init) and switched getdata and viewlibs to it, so neither hardcodes the libref and approvers uses the admin group from the file. test/mock-settings.test.js proves it: it renames the control library and moves the data, and both services follow.

The other 13 services still carry const dcLibref = 'DC_JSLIB' and build paths from it at file scope, so converting them is one service at a time rather than a sweep - worth doing, and I have not claimed it here.

node --test sas/mocks/test/*.test.js - 91 pass, 0 fail (including the golden scan and the new settings test). The sas9 mock keeps its own getdata/viewlibs variant and is untouched; the goldens come from the SASjs Server estate. CI is running on the new head.

Both scenarios are gated now, at `3bb9fb52` - the diffs were the mock's and they are fixed, not accepted. `check.js`: ``` ok public/startupservice/default (8 accepted difference(s)) ok editors/getdata/mpe_tables (1 accepted difference(s)) ok public/viewlibs/default (1 accepted difference(s)) ``` **getdata, from 388 differences to 1.** `cols` dropped the `DESC` key and now takes `LABEL`/`LONGDESC` from the `MPE_DATADICTIONARY` rows for this lib.ds.column, the way `getdata.sas` augments `mp_getcols` (`DSN` is "Dataset Name to be edited" again); `COLTYPE` is blank for the temporal columns. `sasdata` is sorted by the business key, with character values trimmed as the JSON writer writes them and an all-blank value as a single space. `dqrules` is the active `MPE_VALIDATIONS` set unioned with the schema's `NOTNULL` - limited to the columns left in the selection, which is why a `NOT NULL` on `tx_from` never reaches the grid - and the derived columns' `READONLY`, then de-duplicated and sorted by column and rule type, with `X` at the missing value. `sasparams` gained the current-rows clause `mpe_filtermaster` appends and the space-padded blanks. `approvers` is computed from `MPE_SECURITY`'s `APPROVE` rows with the admin group as fallback instead of a hardcoded `sasdemo`. The one remaining difference is `.maxvarlengths[2].MAXLEN`: `mp_getmaxvarlengths` sizes the table's only numeric by the bytes it occupies (3), the mock measures its text (1). `MAXLEN` sizes a grid column. **viewlibs, from 4 to 1.** The mock now answers the shape `%mp_getlibs` does - engine in `LIBRARYID`, blank `ENGINE`, libref as `LIBRARYNAME` - rather than a nicer one, because a mock that improves on the payload is a mock the Cypress suite cannot be trusted against. The remaining difference is the `SASHELP` row, which SAS lists and the mock estate has no equivalent of. **The settings file you mentioned is already there and now has a name for the reader.** `makedata.js` writes `services/settings.js` with `dc_libref`, `dc_admin_group`, `dc_staging_area` and `dcpath` - the JS twin of the `settings.sas` the backend generates in `%mpe_init` - and `dcMockUtils` had a loader. I added `dcInit()` as the one entry point (the mock's `%mpe_init`) and switched `getdata` and `viewlibs` to it, so neither hardcodes the libref and `approvers` uses the admin group from the file. `test/mock-settings.test.js` proves it: it renames the control library and moves the data, and both services follow. The other 13 services still carry `const dcLibref = 'DC_JSLIB'` and build paths from it at file scope, so converting them is one service at a time rather than a sweep - worth doing, and I have not claimed it here. `node --test sas/mocks/test/*.test.js` - 91 pass, 0 fail (including the golden scan and the new settings test). The sas9 mock keeps its own getdata/viewlibs variant and is untouched; the goldens come from the SASjs Server estate. CI is running on the new head.

hermes reviewed 2026-11-07 00:45:31 +00:00

hermes left a comment

Author

Collaborator

Copy link

Copy source

REVIEWED ON 2026-11-07 00:45 UTC

Reviewed the full base...head diff: dcMockUtils dcInit wrapper, getdata.js dqrules/rows/cols/approvers work, viewlibs payload shape, the harness webout stub, new tests, and the parity gate. Not assessed in depth: the 1047-line recorded golden (data only, shape checked).

• sas/mocks/sasjs/services/editors/getdata.js:551 approversForTable reads row.libref, row.dsn, row.access_level, row.sas_group, row.group_name and row.user_name directly, while field() was added in this same PR precisely because the seeded rows carry uppercase keys (DD_SOURCE) and ed-flow rows come back lowercased. On a seeded estate every comparison is against undefined, String(undefined) equals 'UNDEFINED' and the function returns no approvers, silently falling back to dcAdminGroup for every table. Resolve every one of those reads through field() and the finding goes away.
• sas/mocks/sasjs/services/editors/getdata.js:150 and :553 gate rows on Number(r.tx_to) > now. A blank, absent or SAS/ISO-formatted tx_to makes that NaN, so the comparison is false and the row is dropped with no signal: for MPE_VALIDATIONS that means a rule that should be in force disappears, and for MPE_SECURITY the grant is ignored. Decide the representation explicitly (epoch seconds, as Date.now()/1000 assumes) and treat a blank tx_to as open-ended rather than expired, or the mock quietly diverges from getdata.sas whenever a row omits the end date.
• sas/mocks/sasjs/services/editors/getdata.js:14 and sas/mocks/sasjs/services/public/viewlibs.js:13 take dcLibref straight from dcInit() with no fallback, while dcAdminGroup two lines later has one. A fresh deploy that has not run makedata, or a settings.js without dc_libref, yields an undefined libref and every path computed from it becomes undefined - and the viewlibs comment right below asserts the control library must always be listed even when its folder is missing. Give dcLibref the same default ('DC_JSLIB') that the previous constant provided.
• sas/mocks/sasjs/services/editors/getdata.js:585 builds effectiveFilterText by appending the current-rows clause, but sasdata is still returned unfiltered. FILTER_TEXT therefore tells the client a current-rows restriction is in force that the mock never applied, so expired rows are present and labelled as excluded. Either apply the clause when selecting rows or leave FILTER_TEXT to echo what was actually filtered.
• sas/mocks/sasjs/services/editors/getdata.js:534 sorts augmentedRows by pkColumns.map(name => name.toUpperCase()), but the row keys are written from info.name in the original column casing, and the same block reads row[col.name] / row[lcName] elsewhere. A DDL that spells its columns in lowercase leaves every sort key undefined, the comparator returns 0 and the primary-key order getdata.sas guarantees is silently lost. Resolve the key through the same case-insensitive lookup the rest of the file uses.
• sas/mocks/test/parity/README.md:78 states the per-scenario difference counts and then lists the five diffs the first recording produced. Both go stale on the next commit - a count changes with any mock edit, and the list is history that belongs in the commit message. State the invariant (scenarios are gated; a gated:false scenario is advisory until the mock matches) and drop the enumeration.

No blocking issues; the dqrules union/dedupe/sort, the character padding and the harness showmeta stub are consistent with what the recorded golden shows.

REVIEWED ON 2026-11-07 00:45 UTC Reviewed the full base...head diff: dcMockUtils dcInit wrapper, getdata.js dqrules/rows/cols/approvers work, viewlibs payload shape, the harness webout stub, new tests, and the parity gate. Not assessed in depth: the 1047-line recorded golden (data only, shape checked). - sas/mocks/sasjs/services/editors/getdata.js:551 approversForTable reads row.libref, row.dsn, row.access_level, row.sas_group, row.group_name and row.user_name directly, while field() was added in this same PR precisely because the seeded rows carry uppercase keys (DD_SOURCE) and ed-flow rows come back lowercased. On a seeded estate every comparison is against undefined, String(undefined) equals 'UNDEFINED' and the function returns no approvers, silently falling back to dcAdminGroup for every table. Resolve every one of those reads through field() and the finding goes away. - sas/mocks/sasjs/services/editors/getdata.js:150 and :553 gate rows on Number(r.tx_to) > now. A blank, absent or SAS/ISO-formatted tx_to makes that NaN, so the comparison is false and the row is dropped with no signal: for MPE_VALIDATIONS that means a rule that should be in force disappears, and for MPE_SECURITY the grant is ignored. Decide the representation explicitly (epoch seconds, as Date.now()/1000 assumes) and treat a blank tx_to as open-ended rather than expired, or the mock quietly diverges from getdata.sas whenever a row omits the end date. - sas/mocks/sasjs/services/editors/getdata.js:14 and sas/mocks/sasjs/services/public/viewlibs.js:13 take dcLibref straight from dcInit() with no fallback, while dcAdminGroup two lines later has one. A fresh deploy that has not run makedata, or a settings.js without dc_libref, yields an undefined libref and every path computed from it becomes undefined - and the viewlibs comment right below asserts the control library must always be listed even when its folder is missing. Give dcLibref the same default ('DC_JSLIB') that the previous constant provided. - sas/mocks/sasjs/services/editors/getdata.js:585 builds effectiveFilterText by appending the current-rows clause, but sasdata is still returned unfiltered. FILTER_TEXT therefore tells the client a current-rows restriction is in force that the mock never applied, so expired rows are present and labelled as excluded. Either apply the clause when selecting rows or leave FILTER_TEXT to echo what was actually filtered. - sas/mocks/sasjs/services/editors/getdata.js:534 sorts augmentedRows by pkColumns.map(name => name.toUpperCase()), but the row keys are written from info.name in the original column casing, and the same block reads row[col.name] / row[lcName] elsewhere. A DDL that spells its columns in lowercase leaves every sort key undefined, the comparator returns 0 and the primary-key order getdata.sas guarantees is silently lost. Resolve the key through the same case-insensitive lookup the rest of the file uses. - sas/mocks/test/parity/README.md:78 states the per-scenario difference counts and then lists the five diffs the first recording produced. Both go stale on the next commit - a count changes with any mock edit, and the list is history that belongs in the commit message. State the invariant (scenarios are gated; a gated:false scenario is advisory until the mock matches) and drop the enumeration. No blocking issues; the dqrules union/dedupe/sort, the character padding and the harness showmeta stub are consistent with what the recorded golden shows.

hermes added 1 commit 2026-11-07 00:45:56 +00:00

fix(mocks): viewdata sends a blank cell as a space, not the word undefined ... b301399b5c

String(undefined) put the literal text "undefined" into every cell of a column
the row does not carry - the ordinary case in MPE_TABLES, where nine columns are
blank on every row - so the viewer rendered the word instead of an empty cell.

A value the row does not carry is SAS's missing, so a character column gets a
space and a numeric one '.', which is what the empty-table branch beside it
already did.

sas/mocks/test/viewdata.test.js fails without the change and passes with it.

hermes commented 2026-11-07 00:46:02 +00:00

Author

Collaborator

Copy link

Copy source

Asked whether public/viewdata works, so I recorded it the same way (--scenario public/viewdata/mpe_tables --libref DC634301) and replayed it. It runs - no abort, the members the client reads (viewdata, $viewdata) are there - but it is not at parity: 364 differences on MPE_TABLES.

The largest one is a real bug, fixed at b301399b: a cell the row does not carry went through String(undefined), so every blank cell in MPE_TABLES (nine columns are blank on every row) reached the viewer as the word undefined. It now sends SAS's missing - a space for a character column, . for a number - and test/viewdata.test.js fails without the change and passes with it.

What remains, measured after that fix:

• 138 differences, 49 once cols is sorted - SAS returns viewdata's cols in name order, the mock returns them in schema order with the temporal columns first, which alone accounts for 89 of them.
• cols[3].LABEL: same MPE_DATADICTIONARY gap as getdata had ("Dataset Name to be edited" vs "DSN").
• timestamps render as 01JAN1960:00:00:00 where the estate answers 01JAN60:00:00:00.00 - two-digit year and a two-decimal fraction. The format on the column is DATETIME19.3, so the fraction is not simply the format's decimal count; I did not want to guess it into formatSasValue, which four other services share.
• $viewdata types the temporal columns as char / length 200 / $200. where the estate answers num / 8 / DATETIME19.3, because the viewer mock reuses the entry built for the edit grid, where those columns are converted to character.

I left the viewdata scenario out of the commit rather than committing a 49-difference golden as gated: false - it is a work list for its own PR, and the timestamp question needs an answer from the estate rather than a guess. Say the word and I will take it next; the first three are mechanical.

CI is running on b301399b for the branch as a whole.

Asked whether `public/viewdata` works, so I recorded it the same way (`--scenario public/viewdata/mpe_tables --libref DC634301`) and replayed it. It runs - no abort, the members the client reads (`viewdata`, `$viewdata`) are there - but it is not at parity: 364 differences on `MPE_TABLES`. The largest one is a real bug, fixed at `b301399b`: a cell the row does not carry went through `String(undefined)`, so every blank cell in MPE_TABLES (nine columns are blank on every row) reached the viewer as the word **undefined**. It now sends SAS's missing - a space for a character column, `.` for a number - and `test/viewdata.test.js` fails without the change and passes with it. What remains, measured after that fix: - 138 differences, **49 once `cols` is sorted** - SAS returns viewdata's `cols` in name order, the mock returns them in schema order with the temporal columns first, which alone accounts for 89 of them. - `cols[3].LABEL`: same `MPE_DATADICTIONARY` gap as getdata had ("Dataset Name to be edited" vs "DSN"). - timestamps render as `01JAN1960:00:00:00` where the estate answers `01JAN60:00:00:00.00` - two-digit year and a two-decimal fraction. The format on the column is `DATETIME19.3`, so the fraction is not simply the format's decimal count; I did not want to guess it into `formatSasValue`, which four other services share. - `$viewdata` types the temporal columns as char / length 200 / `$200.` where the estate answers num / 8 / `DATETIME19.3`, because the viewer mock reuses the entry built for the *edit* grid, where those columns are converted to character. I left the viewdata scenario out of the commit rather than committing a 49-difference golden as `gated: false` - it is a work list for its own PR, and the timestamp question needs an answer from the estate rather than a guess. Say the word and I will take it next; the first three are mechanical. CI is running on `b301399b` for the branch as a whole.

hermes reviewed 2026-11-07 00:52:50 +00:00

hermes left a comment

Author

Collaborator

Copy link

Copy source

Re-review at b301399b. Delta since 3bb9fb52 is one commit: viewdata's blank-cell handling plus sas/mocks/test/viewdata.test.js. Ran the suite on this head (node --test sas/mocks/test/*.test.js: 92 pass) and the parity check (node sas/mocks/test/parity/check.js: all three scenarios ok). CI at this head: Build / Build-and-ng-test success; Build / Build-and-test-development and Lighthouse Checks still pending, so this is code review only.

New in the delta:

• sas/mocks/test/parity/scenarios.json:84 and :99 - both notes still open with "Recorded, not gated: the mock differs from SAS in ways this scenario shows" and then list divergences 3bb9fb5 removed: a DESC key SAS never emits, missing LABEL/LONGDESC/COLTYPE, unpadded sasdata and no business-key order, hardcoded approvers, LIBRARYID=libref with ENGINE='V9'. The same tree now reports ok editors/getdata/mpe_tables (1 accepted difference(s)) and ok public/viewlibs/default (1 accepted difference(s)). Rewrite each note to what the scenario pins now. Related: the PR body says both goldens carry "gated": false and that check.js labels them ADVISORY, but scenarios.json has no gated key and check.js:56 gates anything not exactly false - as committed they are gated. Add the key or correct the body.
• sas/mocks/sasjs/services/public/viewdata.js:189 - "String(undefined) used to reach the grid as the word "undefined" in every blank cell of an all-but-empty column" is the replaced behaviour, which AGENTS.md bans in comments ("no 'the original bug', 'regression from this fix'"). Keep the contract sentence and drop the history.
• sas/mocks/sasjs/services/public/viewdata.js:209 - the synthetic no-data row still writes '' for a character column (and the comment at :203-204 documents that), while the data rows at :192-193 now write ' ' for the same thing. Pick one representation for a blank character cell across both branches. The new test also covers only the character branch; a numeric column reaching '.' is untested.

Still open from the review at 3bb9fb52, unchanged by this push:

• getdata.js:561 - if (String(row.libref).toUpperCase() !== requestedLibref) continue, and the five reads below it (row.dsn, row.access_level, row.sas_group, row.group_name, row.user_name) bypass the field() helper added in this same PR for exactly this case: on a seeded estate the keys are uppercase, every comparison is against undefined, no group is found and every table falls back to dcAdminGroup.
• getdata.js:153 and :564 - .filter(r => Number(r.rule_active) === 1 && Number(r.tx_to) > now) and if (!(Number(row.tx_to) > now)) continue: a blank or absent end date makes that NaN, so the rule or grant is dropped with no signal. Treat a blank tx_to as open-ended and state the epoch-seconds assumption.
• getdata.js:17 and viewlibs.js:15 - const dcLibref = dcSettings.dcLibref and const dcLibref = dcInit(driveRoot, appLoc).dcLibref have no fallback while dcAdminGroup two lines later does, and viewlibs' own comment says DC_JSLIB must always be listed. A deploy without dc_libref yields an undefined libref and undefined paths.
• getdata.js:594-604 - effectiveFilterText gains the current-rows clause but sasdata is returned unfiltered, so FILTER_TEXT reports a restriction the mock never applied.
• getdata.js:439-440 - const keys = pkColumns.map(name => name.toUpperCase()) sorts on uppercased names while row keys keep the DDL's casing, so a lowercase DDL leaves every sort key undefined and the comparator returns 0.
• sas/mocks/test/parity/README.md:78 - the Status paragraph states per-scenario difference counts and then enumerates the first recording's diffs; both go stale on the next mock change, and the scenarios.json notes above are the live example. State the invariant and let the scenario records carry the detail.

9 findings above for review.

Re-review at b301399b. Delta since 3bb9fb52 is one commit: viewdata's blank-cell handling plus sas/mocks/test/viewdata.test.js. Ran the suite on this head (`node --test sas/mocks/test/*.test.js`: 92 pass) and the parity check (`node sas/mocks/test/parity/check.js`: all three scenarios ok). CI at this head: Build / Build-and-ng-test success; Build / Build-and-test-development and Lighthouse Checks still pending, so this is code review only. New in the delta: - sas/mocks/test/parity/scenarios.json:84 and :99 - both notes still open with "Recorded, not gated: the mock differs from SAS in ways this scenario shows" and then list divergences 3bb9fb5 removed: a DESC key SAS never emits, missing LABEL/LONGDESC/COLTYPE, unpadded sasdata and no business-key order, hardcoded approvers, LIBRARYID=libref with ENGINE='V9'. The same tree now reports `ok editors/getdata/mpe_tables (1 accepted difference(s))` and `ok public/viewlibs/default (1 accepted difference(s))`. Rewrite each note to what the scenario pins now. Related: the PR body says both goldens carry "gated": false and that check.js labels them ADVISORY, but scenarios.json has no `gated` key and check.js:56 gates anything not exactly false - as committed they are gated. Add the key or correct the body. - sas/mocks/sasjs/services/public/viewdata.js:189 - "String(undefined) used to reach the grid as the word \"undefined\" in every blank cell of an all-but-empty column" is the replaced behaviour, which AGENTS.md bans in comments ("no 'the original bug', 'regression from this fix'"). Keep the contract sentence and drop the history. - sas/mocks/sasjs/services/public/viewdata.js:209 - the synthetic no-data row still writes '' for a character column (and the comment at :203-204 documents that), while the data rows at :192-193 now write ' ' for the same thing. Pick one representation for a blank character cell across both branches. The new test also covers only the character branch; a numeric column reaching '.' is untested. Still open from the review at 3bb9fb52, unchanged by this push: - getdata.js:561 - `if (String(row.libref).toUpperCase() !== requestedLibref) continue`, and the five reads below it (row.dsn, row.access_level, row.sas_group, row.group_name, row.user_name) bypass the field() helper added in this same PR for exactly this case: on a seeded estate the keys are uppercase, every comparison is against undefined, no group is found and every table falls back to dcAdminGroup. - getdata.js:153 and :564 - `.filter(r => Number(r.rule_active) === 1 && Number(r.tx_to) > now)` and `if (!(Number(row.tx_to) > now)) continue`: a blank or absent end date makes that NaN, so the rule or grant is dropped with no signal. Treat a blank tx_to as open-ended and state the epoch-seconds assumption. - getdata.js:17 and viewlibs.js:15 - `const dcLibref = dcSettings.dcLibref` and `const dcLibref = dcInit(driveRoot, appLoc).dcLibref` have no fallback while dcAdminGroup two lines later does, and viewlibs' own comment says DC_JSLIB must always be listed. A deploy without dc_libref yields an undefined libref and undefined paths. - getdata.js:594-604 - effectiveFilterText gains the current-rows clause but sasdata is returned unfiltered, so FILTER_TEXT reports a restriction the mock never applied. - getdata.js:439-440 - `const keys = pkColumns.map(name => name.toUpperCase())` sorts on uppercased names while row keys keep the DDL's casing, so a lowercase DDL leaves every sort key undefined and the comparator returns 0. - sas/mocks/test/parity/README.md:78 - the Status paragraph states per-scenario difference counts and then enumerates the first recording's diffs; both go stale on the next mock change, and the scenarios.json notes above are the live example. State the invariant and let the scenario records carry the detail. 9 findings above for review.

hermes added 1 commit 2026-11-07 00:57:07 +00:00

fix(mocks): read the control rows as they are stored, and say what is gated ... f32027fe59

Nine findings from review, all taken.

Row reads (getdata):
- approversForTable reads every column through field(): the security and group
  rows are seeded with UPPERCASE keys and an edited row comes back lower-cased,
  so a direct property read compared undefined with 'UNDEFINED' and found no
  grant - silently falling back to the admin group for every table.
- tx_to is handled by rowInForce(): epoch seconds, which is what Date.now()/1000
  assumes, with an ISO string accepted; a blank, absent or unparseable end date
  is open-ended rather than expired, so a rule or a grant that omits it is no
  longer dropped as NaN. The dqrules filter reads rule_active/base_lib/base_ds
  through field() for the same reason.

Rows and the text that describes them (getdata):
- the current-rows clause in FILTER_TEXT is now applied to the selection: the
  grid no longer shows expired rows while telling the user they were excluded.
- the primary-key sort resolves each key to the column's own casing, so a
  lowercase DDL sorts on real values instead of comparing undefined.

Settings (getdata, viewlibs):
- dcLibref falls back to 'DC_JSLIB' like dcAdminGroup already did: a deploy that
  has not run makedata has no settings.js, and viewlibs must still list the
  control library.

Wording that had gone stale:
- the parity README's Status paragraph now states the invariant (every scenario
  is gated; gated:false is advisory) instead of counts and a list of differences
  that were fixed, and the scenarios.json notes say what each scenario pins now.
- viewdata's comment states the contract rather than the behaviour it replaced,
  and the synthetic no-data row writes the same space for a blank character cell
  as the data rows do.

Tests: getdata-rules.test.js covers an uppercase-keyed APPROVE grant and an
open-ended rule (both fail on the previous code); viewdata.test.js covers the
numeric missing and the empty-table row.

hermes commented 2026-11-07 00:57:22 +00:00

Author

Collaborator

Copy link

Copy source

All nine taken, at f32027fe. The PR body is rewritten too - it still described the advisory state from the first commit.

Row reads (getdata.js)

1. approversForTable reads every column through field() now: libref, dsn, access_level, sas_group, group_name, user_name. Your reading of the failure was right - on a seeded estate each comparison was undefined against a real value, so no grant was found and every table fell back to the admin group.
2. tx_to goes through a rowInForce() helper: epoch seconds (the representation Date.now()/1000 already assumed, and now stated in the comment), an ISO string accepted, and a blank, absent or unparseable end date treated as open-ended rather than expired. rule_active/base_lib/base_ds in the same filter read through field().

Rows and the text describing them

3. FILTER_TEXT's current-rows clause is now applied: rows whose validity variable is in the past are excluded from sasdata, so the text and the grid agree. A row with no end date stays, as the clause implies.
4. The primary-key sort resolves each key to the column's own casing through visibleColumns, so a lowercase DDL sorts on values instead of comparing undefined against undefined.

Settings

5. dcLibref falls back to 'DC_JSLIB' in both getdata and viewlibs, matching the dcAdminGroup fallback beside it - a deploy that has not run makedata has no settings.js, and viewlibs must still list the control library.

Wording

6. parity/README.md Status now states the invariant only: every scenario in scenarios.json is gated, gated: false is advisory until the mock matches, and the counts live in the gate's output.
7. The two scenarios.json notes say what each scenario pins now; both still opened with "Recorded, not gated" and listed divergences this branch fixed. The PR body's gated: false / ADVISORY claim is corrected - as committed they are gated.
8. viewdata.js comment keeps the contract sentence and drops what it replaced, per AGENTS.md.
9. A blank character cell is a space in both branches, including the synthetic no-data row, and viewdata.test.js now covers the numeric missing (. on an empty table) as well as the character one.

Two new tests in getdata-rules.test.js pin 1 and 2, and both fail against the previous code - I checked by running them with getdata.js reverted:

✖ an APPROVE grant whose keys are upper case still yields its group
✖ a rule with no end date is in force, not dropped

node --test sas/mocks/test/*.test.js - 95 pass, 0 fail. check.js - all three scenarios ok.

All nine taken, at `f32027fe`. The PR body is rewritten too - it still described the advisory state from the first commit. **Row reads (`getdata.js`)** 1. `approversForTable` reads every column through `field()` now: `libref`, `dsn`, `access_level`, `sas_group`, `group_name`, `user_name`. Your reading of the failure was right - on a seeded estate each comparison was `undefined` against a real value, so no grant was found and every table fell back to the admin group. 2. `tx_to` goes through a `rowInForce()` helper: epoch seconds (the representation `Date.now()/1000` already assumed, and now stated in the comment), an ISO string accepted, and a blank, absent or unparseable end date treated as open-ended rather than expired. `rule_active`/`base_lib`/`base_ds` in the same filter read through `field()`. **Rows and the text describing them** 3. `FILTER_TEXT`'s current-rows clause is now applied: rows whose validity variable is in the past are excluded from `sasdata`, so the text and the grid agree. A row with no end date stays, as the clause implies. 4. The primary-key sort resolves each key to the column's own casing through `visibleColumns`, so a lowercase DDL sorts on values instead of comparing `undefined` against `undefined`. **Settings** 5. `dcLibref` falls back to `'DC_JSLIB'` in both `getdata` and `viewlibs`, matching the `dcAdminGroup` fallback beside it - a deploy that has not run `makedata` has no `settings.js`, and `viewlibs` must still list the control library. **Wording** 6. `parity/README.md` Status now states the invariant only: every scenario in `scenarios.json` is gated, `gated: false` is advisory until the mock matches, and the counts live in the gate's output. 7. The two `scenarios.json` notes say what each scenario pins now; both still opened with "Recorded, not gated" and listed divergences this branch fixed. The PR body's `gated: false` / `ADVISORY` claim is corrected - as committed they are gated. 8. `viewdata.js` comment keeps the contract sentence and drops what it replaced, per AGENTS.md. 9. A blank character cell is a space in both branches, including the synthetic no-data row, and `viewdata.test.js` now covers the numeric missing (`.` on an empty table) as well as the character one. Two new tests in `getdata-rules.test.js` pin 1 and 2, and both fail against the previous code - I checked by running them with `getdata.js` reverted: ``` ✖ an APPROVE grant whose keys are upper case still yields its group ✖ a rule with no end date is in force, not dropped ``` `node --test sas/mocks/test/*.test.js` - 95 pass, 0 fail. `check.js` - all three scenarios `ok`.

allan merged commit d4fd34b167 into release/7.23 2026-11-07 01:09:32 +00:00

allan deleted branch feat/parity-getdata-viewlibs 2026-11-07 01:09:33 +00:00

hermes reviewed 2026-11-07 01:16:58 +00:00

hermes left a comment

Author

Collaborator

Copy link

Copy source

Re-review at f32027fe. Delta since the review at b301399b is one commit, fix(mocks): read the control rows as they are stored, and say what is gated; the rest of base...head is unchanged. Ran the suite on this head (node --test sas/mocks/test/*.test.js: 95 pass) and the gate (node sas/mocks/test/parity/check.js: all three scenarios ok, 8/1/1 accepted differences). CI at this head: Build / Build-and-ng-test and Lighthouse Checks success, Build / Build-and-test-development pending - code review only.

Closed by this push: the direct reads in approversForTable (now field() throughout), the blank/absent tx_to handling, the dcLibref fallback at getdata.js:17 and viewlibs.js:17, the primary-key sort key casing at getdata.js:487, the FILTER_TEXT/sasdata mismatch (the restriction is now applied to visibleRows), the README status paragraph, and the two rewritten scenarios.json notes.

• sas/mocks/sasjs/services/editors/getdata.js:147-148 - "tx_to is epoch seconds on the seeded rows, which is the representation Date.now()/1000 assumes" is wrong on the fact, and the two comparisons built on it are wrong with it. Control-table datetimes are SAS datetimes, seconds since 1960-01-01: dcMockUtils.js:450-473 (isCurrentOn -> sasDatetimeNow(), and getdata.js evals that file at :7), the OPEN / sasNow() helpers in test/control-tables.test.js:21-26, and the service being mimicked at sas/sasjs/services/editors/getdata.sas:828 (where &dc_dttmtfmt. lt tx_to). getdata.js:170 and :368 compare such a value against Math.floor(Date.now() / 1000), a gap of 315619200 seconds - about ten years - so a row counts as expired only once it has been expired for a decade. Measured on this head with the repo's own harness: an MPE_VALIDATIONS row with TX_TO = sasNow() - 3600 is still returned in dqrules, an MPE_SECURITY APPROVE row with the same TX_TO still contributes its group's approvers, and the identical row carrying TX_TO = unixNow - 3600 is dropped - the comparison is unix while the data is SAS. The same offset defeats the new restriction at :366-378: expired rows stay in sasdata while effectiveFilterText tells the user they were excluded, which is the divergence that block was added to remove. Initialise now at :170 and nowSeconds at :368 from sasDatetimeNow() (already in scope) instead of Date.now()/1000, and use the same at :608. mpe_derived_postedit.js:197-204 carries the same assumption and is worth the same change, though it is not this diff.

• sas/mocks/test/getdata-rules.test.js:54 - the only case touching tx_to asserts the open-ended branch, and nothing seeds a row whose TX_TO is in the past, so neither rowInForce's expired path (:155-165) nor the :366-378 row restriction is exercised - which is why a ten-year offset passed 95 green tests. Add a case with TX_TO = sasNow() - 3600 asserting the rule is absent from dqrules and its row absent from sasdata, plus one asserting FILTER_TEXT and sasdata agree.

2 findings above for review.

Re-review at f32027fe. Delta since the review at b301399b is one commit, `fix(mocks): read the control rows as they are stored, and say what is gated`; the rest of base...head is unchanged. Ran the suite on this head (`node --test sas/mocks/test/*.test.js`: 95 pass) and the gate (`node sas/mocks/test/parity/check.js`: all three scenarios ok, 8/1/1 accepted differences). CI at this head: Build / Build-and-ng-test and Lighthouse Checks success, Build / Build-and-test-development pending - code review only. Closed by this push: the direct reads in approversForTable (now field() throughout), the blank/absent tx_to handling, the dcLibref fallback at getdata.js:17 and viewlibs.js:17, the primary-key sort key casing at getdata.js:487, the FILTER_TEXT/sasdata mismatch (the restriction is now applied to visibleRows), the README status paragraph, and the two rewritten scenarios.json notes. - sas/mocks/sasjs/services/editors/getdata.js:147-148 - "`tx_to` is epoch seconds on the seeded rows, which is the representation Date.now()/1000 assumes" is wrong on the fact, and the two comparisons built on it are wrong with it. Control-table datetimes are SAS datetimes, seconds since 1960-01-01: dcMockUtils.js:450-473 (`isCurrentOn` -> `sasDatetimeNow()`, and getdata.js evals that file at :7), the OPEN / `sasNow()` helpers in test/control-tables.test.js:21-26, and the service being mimicked at sas/sasjs/services/editors/getdata.sas:828 (`where &dc_dttmtfmt. lt tx_to`). getdata.js:170 and :368 compare such a value against `Math.floor(Date.now() / 1000)`, a gap of 315619200 seconds - about ten years - so a row counts as expired only once it has been expired for a decade. Measured on this head with the repo's own harness: an MPE_VALIDATIONS row with `TX_TO = sasNow() - 3600` is still returned in dqrules, an MPE_SECURITY APPROVE row with the same TX_TO still contributes its group's approvers, and the identical row carrying `TX_TO = unixNow - 3600` is dropped - the comparison is unix while the data is SAS. The same offset defeats the new restriction at :366-378: expired rows stay in sasdata while effectiveFilterText tells the user they were excluded, which is the divergence that block was added to remove. Initialise `now` at :170 and `nowSeconds` at :368 from `sasDatetimeNow()` (already in scope) instead of `Date.now()/1000`, and use the same at :608. mpe_derived_postedit.js:197-204 carries the same assumption and is worth the same change, though it is not this diff. - sas/mocks/test/getdata-rules.test.js:54 - the only case touching tx_to asserts the open-ended branch, and nothing seeds a row whose TX_TO is in the past, so neither rowInForce's expired path (:155-165) nor the :366-378 row restriction is exercised - which is why a ten-year offset passed 95 green tests. Add a case with `TX_TO = sasNow() - 3600` asserting the rule is absent from dqrules and its row absent from sasdata, plus one asserting FILTER_TEXT and sasdata agree. 2 findings above for review.

hermes reviewed 2026-11-07 01:17:18 +00:00

hermes left a comment

Author

Collaborator

Copy link

Copy source

Supplement to the review above (Gitea has no review edit - a PUT to the review endpoint answers 405 - so this carries the one thing that could not be corrected in place).

That review was posted after the fact: #491 merged into release/7.23 as d4fd34b at 01:09:32, before any review landed at head f32027fe. The two findings in it therefore need a follow-up commit on release/7.23 before that branch releases, not a change to this PR: the tx_to epoch in getdata.js:170, :368 and :608 reads SAS datetimes as unix seconds (ten years off, measured), and the expired-row path of rowInForce and of the new visible-rows restriction has no test.

Supplement to the review above (Gitea has no review edit - a PUT to the review endpoint answers 405 - so this carries the one thing that could not be corrected in place). That review was posted after the fact: #491 merged into release/7.23 as d4fd34b at 01:09:32, before any review landed at head f32027fe. The two findings in it therefore need a follow-up commit on release/7.23 before that branch releases, not a change to this PR: the tx_to epoch in getdata.js:170, :368 and :608 reads SAS datetimes as unix seconds (ten years off, measured), and the expired-row path of rowInForce and of the new visible-rows restriction has no test.

hermes referenced this pull request 2026-11-07 08:33:34 +00:00

fix(mocks): pad only a character cell the row does not carry #495

hermes referenced this pull request 2026-11-07 08:37:58 +00:00

feat(mocks): record public/viewdata and bring the viewer payload to parity #492

hermes referenced this pull request 2026-11-07 08:38:00 +00:00

feat(mocks): record four more read paths - getcols, an abort, an edit submission and a download #493

hermes referenced this pull request 2026-11-07 08:38:01 +00:00

refactor(mocks): read the control library from the settings file in every service #494

hermes referenced this pull request 2026-11-07 17:41:32 +00:00

feat(release): 7.23 - licence state as a banner, an 849 kB initial load, numbered logs #481

Sign in to join this conversation.

Reviewers

No Reviewers

hermes

Labels

Clear labels

bug

Something is not working

duplicate

This issue or pull request already exists

enhancement

New feature

help wanted

Need some help

invalid

Something is wrong

question

More information is needed

wontfix

This won't be fixed

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

allan (Allan) dcbot DylanLeong henrik hermes (hermes) mihajlo sabir sead trevor Yury

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: dc/dc#491