Repository navigation
spec: Feature-flag gates for analytics and experiment endpoints (#37659) - #37690
Open
freddyDOTCMS wants to merge 35 commits into
Open
freddyDOTCMS wants to merge 35 commits into
freddyDOTCMS wants to merge 35 commits into
Conversation
ref: #29555 Changing link from `/c/dotAI` to `/c/dotai`
6 tasks
Contributor
|
Claude finished @freddyDOTCMS's task in 2m 29s —— View job Claude PR ReviewReviewed the backend gating changes ( New Issues
Notes (non-blocking)
Everything else — the · branch |
freddyDOTCMS
marked this pull request as draft
September 22, 2026 19:30
freddyDOTCMS
marked this pull request as ready for review
September 22, 2026 19:30
…ts (#37659) Defines gate behavior for FEATURE_FLAG_CONTENT_ANALYTICS and FEATURE_FLAG_EXPERIMENTS across EventAnalyticsProxyResource and ExperimentsResource, including event filtering, context stripping, siteauth access rules, and restart-required flag enforcement. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ges, auth ordering - Expand Independent Test in all 4 user stories: add unit tests for gate logic and payload manipulation; add Postman/integration split with explicit rationale for why non-403 scenarios cannot use Postman - Add Contract Changes section declaring new 403 response paths on all affected endpoints and the context.experiments payload mutation - Add auth-ordering rule to FR-001 (auth before gate; 401 not 403) with explicit exception for POST event endpoint (site_auth returns 400) - FR-003a: clarify /health is an existing endpoint, not a new one - FR-010: add Postman test declaration for error envelope shape - FR-011: replace startup-cache language with outcome statement; add rollback-safety note; add integration test declaration - FR-011: change implementation-prescriptive scope to observable outcome - FR-001: add auth-ordering exception for POST /analytics/content/event - Add UI health-check note to FR-002a and FR-003a Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
freddyDOTCMS
force-pushed
the
issue-37659-analytics-experiment-flag-gates
branch
from
September 22, 2026 23:44
07760e6 to
d81e076
Compare
…, flag/gate contradiction
- FR-007: clarify context.experiments stripping is top-level only (shared
across all events in the batch, not per-event)
- FR-009: define whole-batch-reject semantics — if any event has a
non-pageview event_type the entire request is rejected with 403;
rationale: gate is atomic, silent partial drops obscure client bugs
- Key Entities: fix internal contradiction — describe general live-toggle
behavior and explicitly carve out the gate as startup-read-only exception
- FR-003: drop "and health" exception — FR-003a is the single source for
the /health path; FR-003 now covers all paths except siteauth
- FR-003a: clarify that /analytics/health changes from pass-through proxy
to a dotCMS-produced three-state response (OK/NOT_CONFIGURED/
CONFIGURATION_ERROR); call out as intentional scope beyond "add a gate"
- Event Payload entity: correct shape to {"context":{...},"events":[...]}
and note which gate logic operates per-event vs. top-level
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…cs, UI messages - Remove in-flight edge case (moot with startup-only gate reads) - FR-002a: clarify three-state health check already exists; only the 403 gate is new scope for this feature - FR-003a: document Analytics portlet frontend changes required — update health check client to consume OK/NOT_CONFIGURED/CONFIGURATION_ERROR; specify NOT_CONFIGURED and CONFIGURATION_ERROR analytics-specific messages - Legacy Considerations: add accepted-exception note for GET /analytics/health response shape change when flag=ON; defer to FR-003a for rationale - FR-011: state current live-toggle behavior explicitly, then declare gate MUST NOT use it; name divergent state as intentional; call wiring gate into live-refresh path an implementation error; fix MUST→MUST NOT - FR-003: drop stale "and health" exception (FR-003a is single source) - Key Entities: fix internal contradiction — gate reads startup-only, live-toggle preserved for all other consumers Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…cs-experiment-flag-gates
…h model - Gate ordering: pin feature-flag gate before persistenceMode=readonly check on POST /api/v1/analytics/content/event; 403 short-circuits the chain before persistenceMode is evaluated - FR-010: require FEATURE_DISABLED error code on all flag-disabled 403s to distinguish from SITE_ACCESS_DENIED on the same endpoints; add concrete response example and update Postman test assertion - FR-003a: preserve catch-all auth model — backend user required (401), site READ permission required (403 SITE_ACCESS_DENIED); both run before the feature-flag gate and before the health evaluation - FR-002a: clarify existing health check logic is unchanged; only the 403 gate is new scope for this feature Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Clarify that the true→false default change applies to every consumer of both flags across the system, not only the new gate — ensures a new deployment without explicit config is fully disabled with no internal consumers active. Explain why gate-only would create an inconsistent state. Remove implementation-specific class names. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Closes backward-compat scope question from review: no consumers other than the Analytics portlet are known to read GET /api/v1/analytics/health, so the FR-003a response-shape change affects only the portlet, which is updated as part of this feature. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jcastro-dotcms
previously approved these changes
Sep 24, 2026
…o and Arcadio feedback Replace all-or-nothing 403 batch rejection with a partial-success response model that mirrors CAEM's own contract: - POST /content/event never returns 403 for gate reasons; instead returns 202 (success > 0) or 400 (success == 0) with per-event outcomes - New response fields: discarded count, discardedEvents array (gate-filtered events with code: "FEATURE_DISABLED"), re-mapped CAEM error indexes - FR-006: both flags OFF → 400 ERROR, all events in discardedEvents - FR-007: analytics=ON experiments=OFF → strip context.experiments, forward all events; no discardedEvents (stripping is a context mutation) - FR-008: both ON → pass CAEM response through as-is - FR-009: analytics=OFF experiments=ON → per-event filtering; pageview forwarded, non-pageview discarded; 202/400 based on success count - FR-010: scoped to admin endpoints only; ingest uses code: FEATURE_DISABLED in discardedEvents, not the 403 error envelope - FR-011: live-toggle removal extended system-wide to all consumers of both flags, not only the gate; Key Entities updated accordingly - US2 scenario 2 updated to 202 PARTIAL_SUCCESS; scenario 2b added for all-non-pageview batch → 400 ERROR - US3 Independent Test updated: stripping produces no discardedEvents - US4 Independent Test and scenario 1 updated: 400 not 403 for ingest - Added SDK graceful degradation note (out of scope, follow-up task) - Contract Changes section rewritten to document new ingest response shape Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nly gates start/schedule Analytics cannot be enabled without experiments, eliminating the analytics=ON/experiments=OFF state entirely: - Remove User Story 3 (Only Analytics Enabled — impossible state) - Remove FR-007 (context.experiments stripping — only needed for the now-impossible analytics=ON/experiments=OFF case) - Remove Payload mutation section from Contract Changes - Remove SC-003 (analytics-only forwarding criterion) - Remove context.experiments edge case Experiment flag now only gates start/schedule operations: - Rewrite FR-001: only _start and scheduling operations return 403 FEATURE_DISABLED when FEATURE_FLAG_EXPERIMENTS=false; CRUD, results, _end, _cancel, isUserIncluded, and health remain accessible - Update FR-002: all endpoints function normally when flag=true, including start/scheduling - Update FR-002a: health endpoint is always accessible, no longer gated - Update US3 (Both OFF): scenario 2 now reflects only start/schedule blocked; other experiment operations accessible - Update SC-001: experiment start/scheduling blocked, not all endpoints - Renumber FR-008→FR-007, FR-009→FR-008, FR-010→FR-009, FR-011→FR-010 - Renumber SC-004→SC-003, SC-005→SC-004, SC-006→SC-005 - Simplify Event Payload entity: context.experiments forwarded as-is - Update Contract Changes experiments 403 scope to start/schedule only Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…add limited experiment tier
Analytics is now gated by Analytics App configuration (per-site), not a
flag. FEATURE_FLAG_EXPERIMENTS remains as the only flag and now controls
a limited experiment tier rather than a binary on/off gate.
Key changes:
- Remove FEATURE_FLAG_CONTENT_ANALYTICS system-wide; analytics enabled
when Analytics App is configured for the site
- Event ingest gated solely by App configuration (503 when not configured,
forward as-is when configured — regardless of experiments flag)
- Analytics reads return 503 (not 403) when App not configured
- Experiment flag now enables a limited tier: 1 free experiment, max 10
days, _start/_schedule/_abort/archive gated; CRUD/results always accessible
- Add US2: Limited Experiment Mode (flag=false, App configured, free slot)
with real event collection, 10-day UI date picker restriction, and
informational message
- Merge old US2/US3 into single "Limited/Disabled Mode" user story (US3)
- Add Analytics App as a Key Entity; remove FEATURE_FLAG_CONTENT_ANALYTICS
- Analytics health endpoint (GET /analytics/health) always returns
structured OK/NOT_CONFIGURED/CONFIGURATION_ERROR — never proxies raw CAEM
- Experiments health endpoint extended with tier, freeExperimentUsed,
and warning (ANALYTICS_DISABLED) fields
- Free slot count uses {RUNNING, SCHEDULED, ENDED} — ARCHIVED excluded
since archiving is blocked in limited mode
- 400 returned when _start duration exceeds 10 days (not silent cap)
- Remove Independent Test sections from user stories (redundant)
- Update Legacy Considerations, Assumptions, and SC-001–SC-004
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ing test declarations
- FR-009: remove siteauth from 403 scope; add unit test for error body contract
- FR-010: add test-suite audit requirement, scope constraint for live-toggle removal,
FEATURE_FLAG_CAEM_EXPERIMENT_RESULTS and ENABLE_EXPERIMENTS_AUTO_JS_INJECTION left intact
- FR-010: add unit test for default flip (false when flag absent)
- FR-003a: declare health response shape ({ "health": "OK/NOT_CONFIGURED/CONFIGURATION_ERROR" })
- FR-001 / Contract Changes: _end moved to blocked list in all limited-mode states
- FR-006: fix gate ordering (configuration gate → site_auth → persistenceMode → forwarding)
- Key Entities: fix 403 → 503 for analytics read endpoints; fix site_auth ordering note
- Legacy Considerations: acknowledge FR-010 consumer sweep may touch com.dotmarketing.*
- FR-002a: add FEATURE_FLAG_CAEM_EXPERIMENT_RESULTS scope note (new fields apply when true only)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…sponse docs Resolves three documentation gaps found by speckit-converge (Phase 8): - ExperimentsHealthView: remove `allowableValues` from @Schema on `health`, `tier`, and `warning` fields — the declared enum type already provides the values; keeping both caused each value to appear twice in openapi.yaml (e.g. "FULL", "LIMITED", "FULL", "LIMITED"), producing a malformed schema. - ExperimentsHealthView.Tier Javadoc: corrects "full"/"limited" → "FULL"/"LIMITED" to match the actual Jackson-serialized wire format. - ExperimentsResource: add @apiresponse annotations to start(), end(), cancel(), and archive() documenting 200/403/503/400 gate responses — the return type change from ResponseEntitySingleExperimentView to Response left these operations with empty response schemas in openapi.yaml. - Regenerate openapi.yaml after all annotation changes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…R-003/US3/AC4)
GET /api/v1/experiments/{id}/results now returns 503 ANALYTICS_NOT_CONFIGURED
when the Analytics App is not configured for the site. Results depend on the
analytics backend; without it they are meaningless and must not be served.
Also adds @apiresponse documentation for the new 503 response case.
openapi.yaml regenerated.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Pull Request Unsafe to Rollback!!!
Note: no database migrations, Elasticsearch mapping changes, or structural storage changes were found in this PR ( |
…ponse Adds @jsonvalue overrides to ExperimentsHealthView.Tier and Warning so they serialize as "full"/"limited" and "analytics_disabled" instead of the default uppercase enum names. This aligns the wire format with the spec examples in FR-002a and data-model.md, while keeping Java enum names in UPPERCASE per convention. The FE TypeScript types and guards are updated separately (FE PR). openapi.yaml regenerated: tier enum now shows "full"/"limited". Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The Warning enum now serializes as "analytics_disabled" (lowercase via @jsonvalue). Update the Postman test assertion from 'ANALYTICS_DISABLED' to 'analytics_disabled' to match the actual wire format and keep the test consistent with the Tier enum casing ("full"/"limited") agreed in this feature. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… wire values Replaces @jsonvalue method with @JsonProperty on each enum constant: @JsonProperty("full") FULL @JsonProperty("limited") LIMITED @JsonProperty("analytics_disabled") ANALYTICS_DISABLED @JsonProperty is understood by both Jackson (serialization) and swagger-maven- plugin (schema introspection), generating the correct lowercase enum values in openapi.yaml without duplicates. The previous @jsonvalue + allowableValues combination caused each value to appear twice in the schema. Also fixes @Schema descriptions and removes allowableValues overrides — swagger now reads the enum shape directly from @JsonProperty. openapi.yaml: tier ["full","limited"], warning ["analytics_disabled"], each once. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Pull Request Unsafe to Rollback!!!
|
Moves LIMITED_MODE_MAX_DAYS to ExperimentsAPI interface (business layer) so both ExperimentsAPIImpl.startLimitedScheduling() and ExperimentLimitedModeGate reference the same constant. ExperimentLimitedModeGate.MAX_DAYS delegates to ExperimentsAPI.LIMITED_MODE_MAX_DAYS, keeping all limited-mode gate logic in one class while avoiding a reverse dependency (business → REST). ExperimentLimitedModeGate.defaultEndDate() remains the live implementation used by the gate's duration check. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…mited-mode duration - Make ExperimentLimitedModeGate public with MAX_DAYS = 10L and public defaultEndDate() - ExperimentsAPIImpl.startLimitedScheduling() calls ExperimentLimitedModeGate.defaultEndDate(now) instead of duplicating the cap arithmetic — all limited-mode duration logic in one class - ExperimentsAPI.LIMITED_MODE_MAX_DAYS removed — gate is the authority - ExperimentsResource error message uses ExperimentLimitedModeGate.MAX_DAYS Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…any body inspection FR-006 requires the configuration gate to run before any payload inspection. Previously the null-body guard (→400), JSON parse, and context checks all fired before site resolution and the App-config gate (→503). Since getSiteFromRequest() reads only HTTP headers (Origin/Referer), site resolution can be hoisted above the body parse without any loss of information. New order: 1. getSiteFromRequest() — headers only, no body 2. isAppConfigured() → 503 if absent (gate fires before ANY body inspection) 3. body null check → 400 4. JSON parse / context checks → 400 5. site_auth extraction + validation → 400 6. persistence-mode check → 200/forward Also extracts the repeated asyncResponse.resume(400) pattern into resumeWithBadRequest() and the 503 body into analyticsNotConfiguredResponse(), both private static helpers, reducing duplication across the method. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Removes ~35 lines of builder boilerplate. The class becomes a compact record with @JsonInclude(NON_NULL) — Jackson's record support handles nullable fields cleanly without a hand-rolled builder. - Builder replaced by canonical record constructor: new ExperimentsHealthView(health, tier, ...) - Getters replaced by record accessors: view.health(), view.tier(), etc. - Enum definitions and @JsonProperty annotations unchanged - Test accessors updated: getHealth() → health(), getTier() → tier(), etc. - openapi.yaml regenerated (no schema changes, only structural regeneration) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Pull Request Unsafe to Rollback!!!
|
factory.list() loaded all experiment rows on every GET /health and POST /_start call. The new ExperimentsFactory.countByStatuses(Set<Status>) issues a SELECT COUNT(*) … UNION ALL query — one round-trip, no row hydration — and isFreeSlotUsed() returns count > 0. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…s disabled When FEATURE_FLAG_CAEM_EXPERIMENT_RESULTS=false, health is derived from the legacy dotExperiments-config path (AnalyticsHelper + EventLogRunnable test event) rather than ContentAnalyticsUtil.resolveAnalyticsHealth() (CAEM path). The warning field is CAEM-specific and is only populated on the true branch. The response shape (ExperimentsHealthView) is unchanged in both branches. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![semgrep-dotcms[bot]](https://avatars.githubusercontent.com/u/1005263?s=60&v=4){kind=small}
| // Build: SELECT COUNT(*) … WHERE status=? UNION ALL SELECT COUNT(*) … WHERE status=? … | ||
| // then sum the per-status counts in Java — one round-trip, no full-row fetch. | ||
| final StringBuilder sql = new StringBuilder(COUNT_BY_STATUS_BASE); | ||
| statuses.stream().skip(1).forEach(ignored -> sql.append(COUNT_BY_STATUS_UNION)); |
Contributor
There was a problem hiding this comment.
🟡 Medium severity blocking issue identified in your code:
The method identified is susceptible to injection. The input should be validated and properly
escaped.
Why this might be safe to ignore:
The SQL is assembled only from application-controlled constants, while each status value is supplied separately through addParam() as a bound parameter. No caller-controlled input is concatenated into SQL syntax, so this finding is not exploitable.
To resolve this comment:
🔧 No guidance has been designated for this issue. Fix according to your organization's approved methods.
💬 Ignore this finding
Reply with Semgrep commands to ignore this finding.
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
Alternatively, triage in Semgrep AppSec Platform to ignore the finding created by CUSTOM_INJECTION-2.
If this is a critical or high severity finding, please also link this issue in the #security channel in Slack.
You can view more details about this finding in the Semgrep AppSec Platform.
This branch has not been deployed
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
3 participants
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.
Summary
Implements feature-flag and Analytics App configuration gates for experiment activation and analytics proxy endpoints (issue #37659).
Behavior Matrix
FEATURE_FLAG_EXPERIMENTSstatesPOST /_start(immediate)403 FEATURE_DISABLEDPOST /_start(future-dated)403 FEATURE_DISABLED403 FEATURE_DISABLEDPOST /scheduled/_cancel403 FEATURE_DISABLED403 FEATURE_DISABLEDPUT /_archive403 FEATURE_DISABLED403 FEATURE_DISABLEDPOST /_end403 FEATURE_DISABLED403 FEATURE_DISABLEDGET /health200always200always200alwaysisUserIncludedGET /{id}/resultsAnalytics App configuration states (any flag state)
POST /_start503 ANALYTICS_NOT_CONFIGUREDPOST /_cancel,PUT /_archivePOST /_endGET /{id}/results503 ANALYTICS_NOT_CONFIGUREDGET /experiments/healthtier,health,freeExperimentUsed,warningwarning: "analytics_disabled"addedGET /analytics/health200 { health: "ok" }200 { health: "not_configured" }— never 503GET /analytics/{path}(events, etc.)503 ANALYTICS_NOT_CONFIGUREDPOST /analytics/content/event503 ANALYTICS_NOT_CONFIGUREDGET /analytics/content/siteauth/generate/{id}200always200always — never gatedGET /experiments/healthresponse shape (new)tierfreeExperimentUsedwarning"full""full""analytics_disabled""limited"true/false"limited"true/false"analytics_disabled"Key Implementation Details
FEATURE_FLAG_EXPERIMENTSdefault flipstrue→false— new installs start in limited modeConfigExperimentUtil.notify()no longer refreshes the flag; a restart is required for flag changesExperimentLimitedModeGate— public gate class that owns the 10-day cap (MAX_DAYS = 10L) anddefaultEndDate()used by both the resource gate check andExperimentsAPIImpl.startLimitedScheduling()ExperimentsHealthView— new response type withTier("full"/"limited") andWarning("analytics_disabled") enums serialized lowercase via@JsonPropertyGET /analytics/health— changed from raw CAEM proxy to dotCMS-evaluated 3-state response (OK/NOT_CONFIGURED/CONFIGURATION_ERROR)POST /analytics/content/event— App-config check fires beforesite_authvalidation (inverted from the previous order)isFreeSlotUsed()— queries{RUNNING, SCHEDULED, ENDED}experiments globally; no in-memory cache (DB is cluster-shared authority)@JsonPropertyenum values and@ApiResponseannotations on gated endpointsTest Coverage
ConfigExperimentUtilTest,ExperimentsAPIImplTest,ContentAnalyticsUtilTest,ExperimentsResourceTest,EventAnalyticsProxyResourceTest— all greenEventAnalyticsProxyResourceIntegrationTest(real AppsAPI round-trip foranalyticsHealth),ExperimentsResourceIntegrationTest(live-toggle removal verified); both registered inMainSuite2aExperiment_Gatecollection (27 assertions: flag gate 403s, health endpoint fields, analytics 503/200 behavior) — passed against live serverBreaking Change
Pre-ship audit status — COMPLETE: Active customer inventory has been reviewed and confirmed — no customers are currently using the Experiments feature in production. The flag default flip is safe to ship without setting
FEATURE_FLAG_EXPERIMENTS=trueon any customer instance. If that changes before this PR merges, set the flag explicitly for affected customers before deploying.FE Changes
Angular / TypeScript changes are in companion PR #37957 (same feature branch, based on this one). Both PRs must merge together.
🤖 Generated with Claude Code