refactor(remote-config): replace kPreUpdate with batch handler API (DEBUG-4402) #7121
Conversation
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
Collaborator
Author
watson
force-pushed
the
watson/refactor-rc
branch
3 times, most recently
from
b704693 to
eec9001
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7121 +/- ##
==========================================
+ Coverage 84.78% 84.81% +0.02%
==========================================
Files 521 522 +1
Lines 22155 22207 +52
==========================================
+ Hits 18785 18834 +49
- Misses 3370 3373 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Overall package sizeSelf size: 4.36 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 2.0.0 | 68.46 kB | 797.03 kB | | dc-polyfill | 0.1.10 | 26.73 kB | 26.73 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
watson
changed the title
watson
requested review from
khanayan123 and
simon-id
and removed request for
a team
This was referenced Dec 16, 2025
watson
force-pushed
the
watson/refactor-rc
branch
from
eec9001 to
3efdc4c
Compare
watson
force-pushed
the
watson/refactor-rc
branch
from
3efdc4c to
b49c683
Compare
This comment has been minimized.
This comment has been minimized.
- Remove the leaky kPreUpdate hook and introduce a first-class batch API: setBatchHandler()/removeBatchHandler() - Introduce explicit product subscription via subscribeProducts()/ unsubscribeProducts(), and have setProductHandler()/ removeProductHandler() subscribe/unsubscribe for clarity - Centralize ASM/WAF RC product names in appsec/rc-products.js - Update AppSec WAF RC integration to use batched tx (ack/error/markHandled) instead of mutating configs - Improve JSDoc/TS inference for batch transaction/descriptors and WAF manager typing
watson
force-pushed
the
watson/refactor-rc
branch
from
b49c683 to
c18a9fe
Compare
BridgeAR
reviewed
Dec 22, 2025
Collaborator
BridgeAR
left a comment
BridgeAR
left a comment
There was a problem hiding this comment.
It seems like a good improvement to separate things by product. Just left a few minor suggestions
| let appliedActions = new Map() | ||
|
|
||
| /** | ||
| * @param {{ rules?: string, [key: string]: any }} config |
Collaborator
There was a problem hiding this comment.
Nit: I see the improvement, while I would like to prohibit adding any any to the code base
|
|
||
| item.apply_state = ACKNOWLEDGED | ||
| tx.ack(item.path) | ||
| tx.markHandled(item.path) |
Collaborator
There was a problem hiding this comment.
I think we could just implicitly mark it as handled when there is an ack or an error. That way the code is simpler.
| version: conf.version, | ||
| file: conf.file | ||
| } | ||
| return Object.freeze(desc) |
Collaborator
There was a problem hiding this comment.
Do we need to freeze the objects?
|
|
||
| if (toUnapply.length || toApply.length || toModify.length) { | ||
| this.emit(RemoteConfigManager.kPreUpdate, { toUnapply, toApply, toModify }) | ||
| const tx = createUpdateTransaction({ toUnapply, toApply, toModify }, txHandledPaths, txOutcomes) |
Collaborator
There was a problem hiding this comment.
I would strongly favor writing transaction instead of tx.
|
|
||
| this._handlers = new Map() | ||
| this._products = new Set() | ||
| this._batchHandlers = new Map() |
Collaborator
There was a problem hiding this comment.
Please use private properties instead
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.
What does this PR do?
kPreUpdatehook fromRemoteConfigManagerand replaces it with a first-class batch handler API (setBatchHandler/removeBatchHandler) that receives an explicit transaction (ack/error/markHandled).subscribeProducts(...products)/unsubscribeProducts(...products)and makessetProductHandler/removeProductHandlersubscribe/unsubscribe for clarity.appsec/rc-products.js.Motivation
kPreUpdateexposed internal mutable state and blurred responsibility between RC internals and consumers. WAF needs to reconcile multiple products in one logical update, so the new batch API provides a safer and clearer surface.