feat: triggerstp Stored Program - #382
Conversation
Test Coverage ReportOverall coverage: 114/170 macros (67%) Command run: PR-Changed Macros
Notes
Generated by Hermes Agent (GitHub App) |
There was a problem hiding this comment.
Hermes Agent Code Review
Verdict: ❌ Request Changes — The new ms_triggerstp macro contains multiple undefined macro variable references and an incomplete DATA step that will cause runtime failures. The macro appears to be an in-progress adaptation of ms_runstp.sas with several parameters and initializations left out. Given this is a DRAFT PR (body: "WIP"), these findings are expected, but they must be resolved before merge.
Pre-Review Checks
- Mergeable:
false(mergeable_state:dirty) — the PR has merge conflicts withmain. - Check runs: 0 check runs found for the head SHA.
- Coverage:
npx @sasjs/cli cran successfully — 114/170 macros covered (67%).ms_triggerstpis not covered (no test file exists).
Critical Issues
-
&boundaryis never defined — The%localstatement (line 52) omitsboundary, and there is no%let boundary=%mf_getuniquename();. All multipart form-data delimiters (--&boundaryon lines 80, 117, 129, 136) resolve to empty strings, producing a malformed multipart body. The sibling macroms_runstp.sas(line 59) correctly initializes it. -
&outrefis never defined — Used on lines 154, 157, and 168 but never declared as a parameter or%local.ms_runstpdeclaresoutref=outwebas a parameter. Here,proc http out=&outrefwill fail because&outrefresolves to empty. -
&outlogdsis never defined — Used on lines 183 and 196 but never declared. The%if &outlogds ne _null_condition on line 183 references an undefined macro variable.ms_runstpdeclaresoutlogds=_null_as a parameter. -
Incomplete / orphaned DATA step (lines 179–181) —
data work.%mf_getuniquename();starts a DATA step withinfileand a bareinputstatement (no input specification, norun;). Whenoutlogdsis not provided andmdebug=0(the defaults), the%ifblock on line 183 is skipped, leaving an unclosed DATA step — a syntax error. Even when the%ifblock executes, thedata &outlogds; ... run;(lines 196–203) closes a different DATA step; the original one on line 179 is never properly terminated.
Major Issues
-
outdsparameter declared but never used — Theoutds=work.ms_triggerstpparameter (line 49) is documented as the output dataset to contain thesessionId, but no code in the macro body writes to it. The doc header (lines 30–36) describes the expected output format, but the implementation is missing. -
debugparameter accepted but ignored — The URL on line 159 hardcodes_debug=131instead of using&debug. Thedebug=parameter (line 46) has no effect on the request. -
Endpoint path inconsistency — The
@briefdoc (line 3) references/SASjsApi/code/trigger, but the actual URL (line 158) uses/SASjsApi/stp/trigger. These should be consistent; verify which endpoint is correct for the SASjs Server API.
Minor Issues
-
No test file — There is no
tests/serveronly/ms_triggerstp.test.sas. Every otherserver/ms_*.sasmacro that ships has a corresponding test (e.g.ms_runstp.test.sas,ms_testservice.test.sas). A test should be added following the pattern inms_runstp.test.sas. -
all.sascorrectly updated — The concatenatedall.sasfile includes the new macro content matchingserver/ms_triggerstp.sas. No issue here.
Summary
This macro is a near-copy of ms_runstp.sas adapted for the /SASjsApi/stp/trigger endpoint, but the adaptation is incomplete. At minimum, the following must be added/fixed before this is functional:
- Add
%let boundary=%mf_getuniquename();and includeboundaryin%local - Declare
outrefandoutlogdsas parameters (or%localvariables) - Complete the DATA step that parses the response and writes the
sessionIdto&outds - Use
&debuginstead of hardcoded131in the URL - Reconcile the endpoint path in the doc header vs. the URL
- Add a test file
- Resolve merge conflicts with
main
Reviewed by Hermes Agent (GitHub App)
| ,outds=work.ms_triggerstp | ||
| ,mdebug=0 | ||
| ); | ||
| %local dbg mainref authref; |
There was a problem hiding this comment.
🔴 Critical: &boundary is never defined.
The %local statement omits boundary, and there is no %let boundary=%mf_getuniquename();. Compare with ms_runstp.sas line 56-59:
%local dbg mainref authref boundary;
%let boundary=%mf_getuniquename();Without this, all --&boundary references (lines 80, 117, 129, 136) resolve to empty strings, producing a malformed multipart/form-data body. The API request will fail.
|
|
||
| %local resp_path; | ||
| %let resp_path=%sysfunc(pathname(work))/%mf_getuniquename(); | ||
| filename &outref "&resp_path" lrecl=32767; |
There was a problem hiding this comment.
🔴 Critical: &outref is undefined.
&outref is used here and on lines 157, 168 but is never declared as a parameter or %local variable. In ms_runstp.sas, outref=outweb is a declared macro parameter. Here it will resolve to empty, causing filename &outref and proc http out=&outref to fail.
The macro declares outds= instead — but outds is never used in the body (see line 49).
| /* reset options */ | ||
| options &optval; | ||
|
|
||
| data work.%mf_getuniquename(); |
There was a problem hiding this comment.
🔴 Critical: Incomplete / orphaned DATA step.
This data work.%mf_getuniquename(); step has:
infile "&resp_path";(line 180)input(line 181 — bare, no specification)- No
run;statement
When outlogds is undefined (default) and mdebug=0 (default), the %if block on line 183 is skipped entirely, leaving this DATA step unclosed — a SAS syntax error.
This step appears intended to parse the API response and write the sessionId to &outds, but the implementation is missing. The outds parameter is declared (line 49) and documented (lines 30-36) but never populated.
| infile "&resp_path"; | ||
| input | ||
|
|
||
| %if &outlogds ne _null_ or &mdebug=1 %then %do; |
There was a problem hiding this comment.
🔴 Critical: &outlogds is undefined.
&outlogds is used here and on line 196 but is never declared as a parameter or %local variable. In ms_runstp.sas, outlogds=_null_ is a declared parameter. The %if &outlogds ne _null_ condition references an undefined variable (resolves to _null_ by SAS convention, so the block is skipped — but this is accidental, not intentional).
| /* prepare request*/ | ||
| proc http method='POST' headerin=&authref in=&mainref out=&outref | ||
| url="&_sasjs_apiserverurl/SASjsApi/stp/trigger?%trim( | ||
| )_program=&pgm%str(&)_debug=131"; |
There was a problem hiding this comment.
🟡 Major: debug parameter is ignored.
The URL hardcodes _debug=131 instead of using &debug. The debug= parameter (line 46) is accepted but has no effect. Should be:
url="&_sasjs_apiserverurl/SASjsApi/stp/trigger?%trim(
)_program=&pgm%str(&)_debug=&debug";| @@ -0,0 +1,215 @@ | |||
| /** | |||
| @file | |||
| @brief Triggers a SASjs Server STP using the /SASjsApi/code/trigger endpoint | |||
There was a problem hiding this comment.
🟡 Major: Endpoint path inconsistency.
The @brief says /SASjsApi/code/trigger but the actual URL on line 158 uses /SASjsApi/stp/trigger. Please reconcile — verify which is the correct SASjs Server API endpoint and update the doc header to match.
| ,debug=131 | ||
| ,inputparams=_null_ | ||
| ,inputfiles=_null_ | ||
| ,outds=work.ms_triggerstp |
There was a problem hiding this comment.
🟡 Major: outds parameter declared but never used.
The outds=work.ms_triggerstp parameter is documented (lines 30-36) as the output dataset to contain the sessionId, but no code in the macro body writes to it. The response-parsing DATA step (line 179) is incomplete and writes to a random mf_getuniquename() dataset instead.
WIP