A pre-deployed command driver: sync/replicate without S_DEVELOP - #88
Merged
Conversation
Refs #85. The CLI generated a class per command because `erpl-adt object run` takes no parameters. That works, but it needs S_DEVELOP -- which the erpl-rev service user does not have on a production system -- and it turns every value the user types into ABAP source, which is why there is an escaping module with a 22-case injection suite behind it. ZCL_ERPL_REV_CLIDRV is deployed once, by setup, like everything else. The CLI writes a command into a DuckDB table as JSON; the driver claims it, executes it, and writes the result back. Parameters arrive as *data* and are only ever read as values. Nothing is generated, created or deleted. Two ways to reach it, and the second is the point: - `object run ZCL_ERPL_REV_CLIDRV` when the caller may run a classrun. - The periodic Z_ERPL_REV_DELTA heartbeat now drains the queue on each tick. That path involves no ADT call at all, so the CLI needs no SAP authorisation whatsoever -- it writes a row locally and the job in SAP picks it up. When the direct run is refused the command stays queued and the CLI says so, exiting 3: "queued, the job will run it" is a different answer from "done" and is not dressed up as one. Claiming is one UPDATE ... RETURNING, so two drivers racing cannot take the same command -- the same lease idea the delta engine already uses for targets. Three ABAP mistakes worth recording, all mine: - Multi-parameter method calls need named arguments; `jstr( x, 'k' )` is only valid for a single parameter. - zcl_erpl_rev_util=>query returns its rows as a JSON array *string*, not a table, so the driver claims one command per iteration rather than parsing an array. - The JSON reader only handled quoted values, and cmd_id is a number. The driver therefore claimed a command and then decided there was nothing to run -- leaving the row stuck in RUNNING. Silent, and the sort of bug that only shows up as "count=0" when a row plainly exists. Verified live: a replicate queued as JSON loaded T000 through the driver; sync create + sync run went through it and returned real ABAP-side results; no ZCL_ERPL_REV_CLI_* object existed at any point. setup now embeds 15 objects. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All four verbs -- replicate, sync create, sync run, schedule -- now use the pre-deployed driver when it is available, falling back to codegen when it is not. --print-abap and --dry-run still take the codegen path, because printing the ABAP is the point of them. --queue-only is the interesting addition: it writes the command to the local DuckDB and returns, contacting SAP not at all. It does not even resolve credentials, because prompting for a password to write a row into a local database would be absurd. Exit 3, since "queued" is not "done". Proven end to end on A4H, which is the acceptance criterion from #85: $ env -u SAP_USER -u SAP_PASSWORD erpl-rev replicate \ --table T000 --target t000_q --queue-only --yes Queued as command 1. The periodic ERPL_REV_DELTA job will run it. (one Z_ERPL_REV_DELTA tick later) cli 1 replicate DONE rows=2;seconds=0.000 Two rows landed in t000_q, and the command row reads DONE. No SAP credentials, no ADT call, no S_DEVELOP, nothing created or deleted. On the abandoned approach: I first tried to build a PFCG role granting ADT but not S_DEVELOP, to test the direct `object run` path as a restricted user. S_A.SCON does not reach ADT, and assembling a role that does -- without dragging S_DEVELOP in with it -- is real authorisation engineering. The queue path is both cheaper to prove and a stronger claim, since it needs no authorisation at all, so that is what is tested. The throwaway user was deleted and its absence confirmed. What `object run ZCL_ERPL_REV_CLIDRV` requires of a restricted user is therefore still unmeasured; it is an optimisation for latency, not the thing that makes this usable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Closes the gap between "it works when I run it" and "it is checked". - test_cmd_queue.cpp models the ABAP JSON reader independently of the C++ builder and asserts a value survives all three hops it makes: JSON, a SQL string literal, and the driver's parser. Includes the payload that would inject a second key if the escaping were wrong, and the bare numeric scalar that cmd_id arrives as -- the case that silently left a command stuck in RUNNING. - The e2e now queues a replicate with SAP_USER, SAP_PASSWORD and ERPL_REV_SAP_PASSWORD all stripped from the environment, then runs one Z_ERPL_REV_DELTA tick and asserts the command reached DONE and the rows landed. That is the acceptance criterion from #85, executed rather than described. - The docs said sync/replicate need S_DEVELOP. They no longer do once the driver is deployed, and saying otherwise would send people to grant a developer authorisation to a service account for no reason. Two things the e2e caught that manual testing had not: - A report writing a list cannot be SUBMITted from a classrun without capturing the list to memory; plain AND RETURN dumps. - The leaked-object sweep matched ZCL_ERPL_REV_CLI*, which now includes the permanent ZCL_ERPL_REV_CLIDRV -- so it failed on the driver it was meant to ignore. The temp classes are ZCL_ERPL_REV_CLI_<kind><nonce>; the underscore is the distinction. 16804 assertions; e2e 14/14. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
Closes #85.
The problem
The CLI generated an ABAP class per command, because
erpl-adt object runexecutes a class that takes no parameters. Two costs:
S_DEVELOP— a developer authorisation the erpl-rev service userdoes not have on production. The SAP GUI workflow it mirrors needs only
S_PROGRAM.escaping module with a 22-case injection suite behind it.
What changed
ZCL_ERPL_REV_CLIDRVis deployed once, bysetup, like everything else. The CLIwrites a command into the DuckDB table
_erpl_rev_cli_cmdas JSON; the driverclaims it, executes it, writes the result back. Parameters arrive as data and
are only ever read as values. Nothing is generated, created or deleted in SAP.
All four verbs —
replicate,sync create,sync run,schedule— use it whenit is available, falling back to codegen when it is not.
--print-abapand--dry-runstill take the codegen path, since printing the ABAP is their point.Claiming is one
UPDATE … RETURNING, so two drivers racing cannot take the samecommand — the lease idea the delta engine already uses for targets.
The part that actually removes the authorisation
--queue-onlywrites the command and returns, contacting SAP not at all. Itdoes not even resolve credentials — prompting for a password to write a row into
a local database would be absurd. The periodic
ERPL_REV_DELTAjob, which isalready the supported way to run delta on a schedule, drains the queue on each
tick. Exit 3, because "queued" is not "done" and should not be dressed as one.
Proven on A4H, and asserted in the e2e:
Verification
Unit: 16804 assertions, 154 cases.
test_cmd_queue.cppmodels the ABAP JSONreader independently of the C++ builder and asserts a value survives all three
hops — JSON, a SQL string literal, the driver's parser — including the payload
that would inject a second key if the escaping were wrong.
Live: the e2e queues with the credentials stripped from the environment, runs one
heartbeat tick, and asserts the command reached
DONEand the rows landed.e2e 14/14.
Bugs found while building it
Three of my own ABAP mistakes, two of them silent:
jstr( x, 'k' )is onlyvalid for one parameter.
zcl_erpl_rev_util=>queryreturns rows as a JSON array string, not atable, so the driver claims one command per iteration.
cmd_idis a number. Thedriver claimed a command and then decided there was nothing to run, leaving
the row stuck in
RUNNING— visible only ascount=0when a row plainlyexisted.
And two the e2e caught that manual testing had not: a report writing a list
cannot be
SUBMITted from a classrun without capturing the list; and theleaked-object sweep matched
ZCL_ERPL_REV_CLI*, which now includes the permanentCLIDRV, so it failed on the very object it was meant to ignore.Docs
security.md,INSTALL.mdand the README saidsync/replicateneedS_DEVELOP. They no longer do once the driver is deployed, and leaving thatin would send people to grant a developer authorisation to a service account for
no reason.
setupstill needs it, and still says so.Not done
What
object run ZCL_ERPL_REV_CLIDRVrequires of a restricted user is stillunmeasured. I started building a PFCG role granting ADT but not
S_DEVELOPandabandoned it:
S_A.SCONdoes not reach ADT, and assembling one that does withoutdragging
S_DEVELOPin is real authorisation engineering. The queue path needsno authorisation at all, so it is both cheaper to prove and the stronger claim —
that is what is tested. The direct path is a latency optimisation.
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.