feat: LLMO-2805 Stage config addition api changes#1370
Conversation
|
This PR will trigger a minor release when merged. |
nit23uec
left a comment
There was a problem hiding this comment.
knowledge about staging behavior leaking to servlet.
- Add isStageDomain metadata handling in createMetaconfig and updateMetaconfig
- Automatically set prerender: { allowList: ['/*'] } when isStageDomain is true
- Persist isStageDomain as user-defined metadata in S3
- Add tests for staging domain behavior in both create and update methods
This change keeps staging-specific knowledge from leaking to the servlet layer,
maintaining proper separation of concerns between layers.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Review comments have been incorporated:
|
There was a problem hiding this comment.
Hey @jindaliiita
What was done well
The author responded constructively to nit23uec's feedback - moving staging-aware prerender logic out of the API service and into the Tokowaka client. This is directionally the right call: the API service should not know what "staging" means at the metaconfig level. Test coverage is solid with positive and negative cases for both create and update paths. The config schema extension follows the existing codebase patterns (matches cdnLogsConfig.filters, llmo.customerIntent array-of-objects style).
Hold: DynamoDB Schema Change vs. PostgreSQL Migration
The stagingDomains addition to edgeOptimizeConfig in config.js extends the DynamoDB-backed Joi schema. With the Mysticat/PostgreSQL migration landing within a week, this field will need to be re-implemented in the new data service. Recommendation: Consider splitting this PR - land the Tokowaka client changes now, defer the stagingDomains config schema to go directly into PostgreSQL.
Critical
1. updateMetaconfig does not read existing S3 metadata - nit23uec's open comment is a real gap
fetchMetaconfig (line 237-268) reads only the JSON body from S3 - it never reads response.Metadata. This means updateMetaconfig has no way to know a domain was originally created as staging unless the caller re-passes isStageDomain: true every time. The existing API service caller in llmo.js passes { lastModifiedBy } as metadata - never isStageDomain. Every update call will silently lose the staging override context.
The saving grace is that the prerender value survives in the JSON body via existingMetaconfig.prerender. But if someone explicitly passes a different options.prerender, the staging guard is completely bypassed with no warning.
Recommendation: Either (a) persist isStageDomain inside the metaconfig JSON body (not just S3 metadata) so it survives round-trips, or (b) augment fetchMetaconfig to return response.Metadata alongside the body and merge it into the update flow.
2. isStageDomain placed in wrong parameter - category confusion
In the existing codebase, metadata is an opaque pass-through to S3 object metadata for audit/tracing purposes (e.g., last-modified-by). This PR uses it to control business logic (prerender configuration). This is a conceptual mismatch. The behavioral flag should be in options (alongside tokowakaEnabled, enhancements, forceFail, prerender), with storage handled separately.
3. Dual state between stagingDomains (config) and isStageDomain (metadata) with no linking code
The PR adds stagingDomains in the site config schema and isStageDomain in Tokowaka metadata - but nothing connects them. The caller must independently: (1) store staging domains in the config, and (2) pass isStageDomain: true when calling Tokowaka. These can easily drift out of sync. Ideally the Tokowaka client would derive staging status from the config's stagingDomains list rather than trusting a separate caller-provided flag.
Important
4. S3 metadata boolean/string type mismatch
S3 user-defined metadata values are always strings. isStageDomain: true (boolean) becomes "true" (string) in S3. The strict check metadata.isStageDomain === true works at the JS layer before upload, but if fetchMetaconfig is later fixed to read S3 metadata (per finding #1), the string "true" will fail the === true check. This is a latent bug waiting to bite.
5. Hardcoded { allowList: ['/*'] } duplicated and not configurable
The magic value appears in both createMetaconfig (line 323) and updateMetaconfig (line 371). Should be extracted to a constant. Also, isStageDomain in the update path forcibly overrides any existing prerender config - there is no way for a staging domain to have a custom, more restrictive prerender.
6. No lifecycle path for staging-to-production promotion
If a staging domain is promoted to production, isStageDomain remains in S3 metadata, the wildcard prerender persists in the JSON body, and stagingDomains in the config is not cleaned up. None of this has an automated removal path.
7. Tests assert getSlackConfig() - copy-paste artifact
Two tests (lines 2648-2678) named "should preserve provided data if stagingDomains item is missing domain/id" include expect(config.getSlackConfig()).to.be.undefined which is unrelated to the scenario. These test the error-recovery path (Config swallows Joi errors) but the names suggest validation behavior. Should be renamed and the slack assertion removed.
8. Missing test: updateMetaconfig with isStageDomain: false
Create tests cover true, false, and absent. Update tests only cover true and absent - no explicit false test.
9. Domain field lacks format validation
stagingDomains[].domain accepts any string. Other domain/URL fields in the same file use Joi.string().uri(). Should use .hostname() or a regex pattern at minimum. The array also has no .max() limit.
Minor
- No dedicated accessor like
getEdgeOptimizeStagingDomains()(minor inconsistency withgetLlmoBrand()etc.) - TypeScript declarations in
index.d.tsnot updated forcreateMetaconfig/updateMetaconfigsignatures - PR description is boilerplate with no linked issues (title mentions LLMO-2805 but body is empty)
- Commit message
test: tests changedis not descriptive (acceptable if squash-merging)
Recommended Actions
| Priority | Action |
|---|---|
| Block | Defer stagingDomains config schema change - go directly to PostgreSQL instead of adding to DynamoDB |
| Block | Address nit23uec's open comment: persist isStageDomain in metaconfig body or read S3 metadata in fetchMetaconfig |
| Block | Move isStageDomain from metadata to options parameter |
| Before merge | Extract STAGE_DOMAIN_PRERENDER constant, fix copy-paste test assertions, add missing isStageDomain: false update test |
| Follow-up | Design the staging-to-production promotion path, add domain format validation |
Critical fixes from PR review #3860846864: - Fixed fetchMetaconfig to preserve S3 metadata on updates - Added backward compatibility wrapper to prevent API regressions - Fixed null check to validate both wrapper and metaconfig content - Corrected isStageDomain parameter location in JSDoc - Updated all tests to match metadata parameter usage Deferred changes: - Removed stagingDomains from config schema (will be added directly to PostgreSQL in separate PR per reviewer recommendation) All tests passing: 574 (tokowaka-client), 1515 (data-access) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Review Feedback Addressed ✅@solaris007 Thank you for the detailed review! I've addressed all the critical issues: ✅ Critical Issues Fixed1. fetchMetaconfig doesn't return S3 metadata (Issue #1)
2. Null check gap
3. S3 metadata boolean/string handling
4. Documentation fixes
5. Implementation details
📦 Deferred to PostgreSQL PRPer your recommendation:
Removed from this PR:
This will be added directly to PostgreSQL in a separate PR to avoid dual implementation. 🧪 Test Results
Ready for re-review! 🚀 |
solaris007
left a comment
There was a problem hiding this comment.
Re-review: Updates address all blocking issues
All three blockers from the previous review have been resolved:
-
DynamoDB schema deferred to PostgreSQL -
stagingDomainsconfig change and related tests removed from this PR. Will go directly to PostgreSQL in a separate PR. -
S3 metadata now read in update flow - New private
#fetchMetaconfigWithMetadata()returns both the metaconfig body and S3 metadata.updateMetaconfigcorrectly readsexistingS3Metadata.isstagedomain === 'true'to preserve staging domain behavior across updates without requiring callers to re-pass the flag. PublicfetchMetaconfig()preserved for backward compatibility. -
S3 metadata type handling fixed -
isStageDomainpersisted as string'true', read back with lowercase key comparison (isstagedomain- S3 lowercases user-defined metadata keys). No more boolean/string mismatch.
Additional improvements
- Added
isStageDomain: falsetest forupdateMetaconfig - Updated JSDoc with full
metadataparameter documentation - Null check on
#fetchMetaconfigWithMetadataresult
Minor items for follow-up (non-blocking)
- Extract
{ allowList: ['/*'] }to a named constant (duplicated in create and update paths) - Design the staging-to-production promotion lifecycle (cleanup of
isStageDomainmetadata and wildcard prerender) - Consider whether
isStageDomainbelongs inoptionsvsmetadatalong-term
## [@adobe/spacecat-shared-tokowaka-client-v1.10.0](https://github.com/adobe/spacecat-shared/compare/@adobe/spacecat-shared-tokowaka-client-v1.9.0...@adobe/spacecat-shared-tokowaka-client-v1.10.0) (2026-02-26) ### Features * LLMO-2805 Stage config addition api changes ([#1370](#1370)) ([0789f4a](0789f4a))
|
🎉 This PR is included in version @adobe/spacecat-shared-tokowaka-client-v1.10.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Please ensure your pull request adheres to the following guidelines:
Related Issues
Thanks for contributing!