added forcedTargetPath

[ZhengXinCN]

a new feature added called forcedTargetPath, it allows overwrite the listener target path to make the origin requests which have different IDs into the same destination, example can let request from multiple client which have client id in request into the same queue

[mcweba]

Hi there, I had the suspicion that this forcedTargetPath feature strongly relates to the already existing fullUrl option. I will add the output of the AI supported code review. The first part are some findings related to the added code. More important is part 2 where a comparison with the existing fullUrl option is made. I strongly suggest to take a good look at this because it does not make sense to implement new stuff when it can be done with existing features (maybe some changes needed)

[mcweba]

Code Review — forcedTargetPath (PR #787)

Branch	MSOP-8947-allow-override-the-target-path
Commits reviewed	dd15b1d0 (added forcedTargetPath), 0803819d (updated example)
Base	538fe10d
Scope	5 files, +126 / −4
PR state	OPEN — already approved by srudin, commented by ZhengXinCN
PR goal (verbatim)	"a new feature added called forcedTargetPath, it allows overwrite the listener target path to make the origin requests which have different IDs into the same destination, example can let request from multiple client which have client id in request into the same queue"

Link paths in this document are repo-relative so they resolve on GitHub and for other reviewers.

---

Part 1 — Code Review

The 6-line core change works for the happy path, but it has three unguarded failure modes and zero test coverage.

---

🔴 Critical

1. HookHandler.java:876-902: resource_path is overwritten with the forced path

gateleen-hook/src/main/java/org/swisspush/gateleen/hook/HookHandler.java#L876-L902

Code:

String path = request.uri();
if (!listener.getHook().isFullUrl()) {
    path = request.uri().replace(listener.getMonitoredUrl(), "");
}

if (StringUtils.isNotEmptyTrimmed(listener.getHook().getForcedTargetPath())) {
    path = listener.getHook().getForcedTargetPath();   // ← clobbers the derived path
}
...
queueHeaders.add(RESOURCE_PATH, listener.getMonitoredUrl() + path);   // ← now uses the forced path

Risk: A listener exists to answer "what changed?". Once the target URI collapses, resource_path is the only remaining carrier of that information — and this change destroys it too. With the README's own example, all three PUTs arrive at the consumer with:

resource_path: /gateleen/from/services/orders/all/data

…a resource that does not exist and was never written. The consumer cannot distinguish 123 from 456 from 789, and cannot even re-fetch the changed resource. This is silent data loss, not a failed request — nothing logs, nothing 4xx's.

Smallest Fix — keep the derived path for the header, force only the target:

String path = request.uri();
if (!listener.getHook().isFullUrl()) {
    path = request.uri().replace(listener.getMonitoredUrl(), "");
}
String resourcePath = path;                                   // ← add

String targetPath = StringUtils.isNotEmptyTrimmed(listener.getHook().getForcedTargetPath())
        ? listener.getHook().getForcedTargetPath()
        : path;
// ... use targetPath for targetUri
queueHeaders.add(RESOURCE_PATH, listener.getMonitoredUrl() + resourcePath);   // ← unchanged semantics

Missing context: resource_path has exactly one producer and no consumer inside this repo (grep confirms), so impact is only provable against the actual downstream consumers. If losing origin information is genuinely intended, it must be stated in the README — right now it isn't mentioned at all.

counter argument: If every consumer of this particular listener re-reads a collection anyway, collapsing is harmless. But the property is global per hook, not opt-in per header, so nobody can choose otherwise.

---

2. HookHandler.java:888-896: no leading-slash validation → silently corrupted target URI

gateleen-hook/src/main/java/org/swisspush/gateleen/hook/HookHandler.java#L888-L896

Code:

if (listener.getHook().getDestination().startsWith("/")) {
    targetUri = listener.getListener() + path;              // raw concatenation
} else {
    targetUri = hookRootUri + LISTENER_HOOK_TARGET_PATH + listener.getListener() + path;
}

Risk: There is no slash normalization here (unlike Forwarder.buildTargetUri, which does .replaceAll("//", "/")). The schema only enforces minLength: 1, so any string is accepted:

forcedTargetPath	resulting targetUri
orders/all/data (no leading /)	/gateleen/to/services orders/all/data
http://other/x	/gateleen/to/serviceshttp://other/x
destination /a/b/ + /orders	/a/b//orders

Previously path was machine-derived and always well-formed. Now it is hand-written config with zero guard rails. The failure surfaces as permanently-failing queue items, far from the misconfiguration. The README says the value "should be a destination-relative path" — advisory text is not a constraint.

Smallest Fix — enforce it in gateleen_hooking_schema_hook#L71-L75:

"forcedTargetPath": {
    "description": "Only used for listener hooks. Overrides the request path used when forwarding the listener request.",
    "type": "string",
    "pattern": "^/[^?#]*$"
}

This also removes the minLength: 1 vs. isNotEmptyTrimmed mismatch — today " " passes the schema and is then silently ignored by the code, i.e. accepted config with the feature quietly off.

---

3. Zero test coverage — and the harness already exists

Neither commit adds a test. That is not a "nice to have" here, because the harness is already sitting in the file:

HookHandlerTest.java#L262-L286 already does exactly the assertion this feature needs — register a listener via setListenerStorageEntryAndTriggerUpdate(...), fire a PUTRequest, then Mockito.verify(...) on the resulting targetUri. A forcedTargetPath test is ~15 lines of copy-and-adjust.

Likewise HookSchemaTest.validMaximalHook enumerates every hook property — the new one was not added, so the schema change is untested as well.

Without either, a regression in this path is completely invisible to CI. Both findings above (#1 wrong resource_path, #2 malformed URI) would be caught by a single test asserting targetUri and the resource_path header.

---

4. Schema accepts forcedTargetPath on route hooks, where it does nothing

One JsonSchema instance validates both listener and route registrations (HookHandler.java#L1298), and the schema is "additionalProperties": false. Before this change, forcedTargetPath on a route hook was rejected with a validation error. Now it validates cleanly and is silently ignored — setForcedTargetPath is called only in registerListener (line 1521), never in the route registration path.

The schema description says "Only used for listener hooks", which is documentation, not enforcement. This is the same pattern as the existing collection / listable route-only properties, so it is at least consistent — but those degrade to a no-op feature, whereas this one degrades to a no-op routing rule, which is much easier to misdiagnose.

Smallest Fix: a log.warn in the route registration path when FORCED_TARGET_PATH is present.

---

Bottom line: #1 and #2 are shipping blockers — one loses origin information silently, the other lets a typo in config produce a permanently-broken target with no error. #3 is what would have caught both. The feature concept itself is sound and the implementation is appropriately minimal.

---

Part 2 — forcedTargetPath vs. the existing fullUrl feature

Question: fullUrl was added recently. How does forcedTargetPath differ — can the wanted behaviour be achieved with fullUrl?

Short answer: partially — and the part that works reveals a bug in this PR.

fullUrl means two different things in two different places, and which one applies depends on whether your destination is internal or external.

---

The two fullUrl semantics

1. Listener-side — HookHandler.callListener, ancient (traces back to 3927a112, issue #57):

String path = request.uri();
if (!listener.getHook().isFullUrl()) {
    path = request.uri().replace(listener.getMonitoredUrl(), "");
}

fullUrl: true = do not strip the monitored prefix → the appended suffix gets longer. This never collapses anything.

2. Forwarder-side — Forwarder.buildTargetUri, brand new (6adef3f9, #758 — "Implement fullUrl property handling for route hooks"):

if (fullUrl) {
    return rulePath.replaceAll("//", "/");   // request URI ignored entirely
}

fullUrl: true = forward to the exact destination, discard the suffix → this is collapsing.

The README already carries both contradictory definitions:

• README_hook.md#L31 (route table, from Path rewriting control for route hooks #758): "forwarded to the exact destination URL without appending any path suffix"
• README_hook.md#L175 (listener table, pre-existing): "forwards using the full initial url or only the appendix"

Opposite meanings, same property name.

---

Which one applies to a listener?

registerListener branches on the destination:

if (hook.getDestination().startsWith("/")) {
    target = hook.getDestination();          // internal → NO Route, NO Forwarder
} else {
    routeRepository.addRoute(urlPattern, createRoute(urlPattern, hook, requestUrl));  // external → Forwarder
}

And Route.createForwarder does forwarder.setFullUrl(httpHook.isFullUrl()) — so external listeners inherit the #758 collapsing behaviour, even though #758 was written for route hooks.

destination	can fullUrl: true collapse?	why
external (http(s)://…)	✅ yes, already works today	queued request is re-routed through Forwarder, which returns rule.getPath() verbatim
internal (/…)	❌ impossible	no Forwarder is created; targetUri = destination + path and path is never empty. fullUrl: true makes it longer, not shorter

---

So: can you replace forcedTargetPath?

For external destinations — yes, completely. Bake the fixed path into destination:

{
  "destination": "http://target:7012/gateleen/to/services/orders/all/data",
  "fullUrl": true,
  "filter": "/gateleen/from/services/orders/[^/]+/data"
}

Every matching request collapses onto that exact URI. Identical outcome, zero new config properties.

For internal destinations — no. And the README example in this PR uses "destination": "/gateleen/to/services" — i.e. precisely the case fullUrl cannot cover. That is the genuine gap this feature fills.

The only existing workaround for internal is to make the destination an external loopback URL (http://localhost:7012/...) + fullUrl: true. That works, but Route.createHttpClient then builds a real vertx.createHttpClient(...) instead of reusing selfClient — you pay a real TCP round-trip, a separate connection pool, and the full routing/auth chain. Not equivalent.

---

🔴 5. forcedTargetPath is silently discarded on external destinations with fullUrl: true

Because the Forwarder runs after callListener:

config (external destination)	result
fullUrl: false + forcedTargetPath	works — rulePath + forcedTargetPath
fullUrl: true + forcedTargetPath	forcedTargetPath ignored — Forwarder returns rulePath and throws the suffix away

The README claims forcedTargetPath "takes precedence over fullUrl" (README_hook.md#L176). That is true only for internal destinations. For external ones it is exactly backwards — and it fails silently, with no log line.

Smallest Fix: reject the combination at registration time, or at minimum log.warn when both are set on an external destination.

---

Recommendation

The cleanest resolution is not a new property — it's making fullUrl mean one thing. In callListener, honour the #758 semantics for internal destinations too:

String path;
if (listener.getHook().isFullUrl()) {
    path = "";                                                   // exact destination, like Forwarder
} else {
    path = request.uri().replace(listener.getMonitoredUrl(), "");
}

Then the PR's use case is just destination: "/gateleen/to/services/orders/all/data" + fullUrl: true — consistent across route hooks, external listeners and internal listeners, no third path-manipulation knob, and no resource_path corruption (finding #1 disappears, since you'd fix that separately and deliberately).

The trade-off, and it's the real decision: this is a breaking change for any existing internal listener running fullUrl: true, which today gets destination + full-request-URI.

Missing Context: I cannot tell from this repo how many such listeners exist in production, or whether that ancient behaviour was ever intentional rather than an artifact. If it is genuinely in use, then forcedTargetPath is the right call — but it should then be documented as "internal destinations only", and rejected (not silently ignored) when combined with fullUrl: true or with an external destination.

---

Findings index

#	Severity	Finding
1	🔴 Critical	resource_path overwritten with the forced path — origin resource information lost
2	🔴 Critical	No leading-slash validation → silently corrupted target URI
3	🟡 Significant	Zero test coverage — existing harness makes it a ~15-line test
4	🟡 Significant	Schema accepts forcedTargetPath on route hooks, silently ignored
5	🔴 Critical	forcedTargetPath silently discarded on external destination + fullUrl: true; README precedence claim is backwards

[mcweba]

See comments made before

[ZhengXinCN]

@mcweba from document the fullUrl should does what I needed, but it doesn't (you can see from code). so for safety and backward compatibility, I decide to use a new property for this

[mcweba]

@mcweba from document the fullUrl should does what I needed, but it doesn't (you can see from code). so for safety and backward compatibility, I decide to use a new property for this

Personally, I think this is not how we should work. Instead of reusing/extending/fixing an existing feature just create a new similiar feature is bad software engineering. For safety and backward compatibility issues we have unit tests which also can be extended (if needed) to ensure this.

[mcweba]

See my last comment. I like to discuss this first before doing another code review for the forcedTargetPath feature

[srudin]

@mcweba I have sent you a meeting request to discuss this next Friday.