Wip/add decision point to the assign call

[bcb37]

No description provided.

[Copilot]

Pull request overview

Adds optional decision-point scoping (site/target) to the /assign flow so clients can fetch (and cache) assignments either for the full context or for a specific decision point, updating backend services and multiple client SDKs accordingly.

Changes:

• Backend: plumbs site/target through assign controllers → assignment service → experiment fetch/caching → repository query.
• Client SDKs (Python/JS/Java): add request fields + decision-point fetch semantics and merge decision-point results into local caches.
• Tests: extend SDK unit tests and backend integration tests to cover the new parameters and caching behavior.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
packages/backend/test/integration/utils/index.ts	Updates integration test helper to match new getAllExperimentConditions signature.
packages/backend/test/integration/UserNotDefined/index.ts	Updates integration test to pass new parameters to getAllExperimentConditions.
packages/backend/src/api/services/ExperimentService.ts	Adds site/target-aware experiment fetch selection under caching.
packages/backend/src/api/services/ExperimentAssignmentService.ts	Threads site/target through assignment flow into experiment retrieval.
packages/backend/src/api/controllers/validators/ExperimentAssignmentValidator.ts	Extends assign request validator to include site/target.
packages/backend/src/api/controllers/ExperimentClientController.v6.ts	Passes site/target into assignment service for v6 assign endpoint.
packages/backend/src/api/controllers/ExperimentClientController.v5.ts	Passes site/target into assignment service for v5 assign endpoint.
clientlibs/python/tests/test_types.py	Adds AssignRequest test coverage for decision-point fields.
clientlibs/python/tests/test_data_service.py	Adds tests for decision-point cache upsert/merge behavior.
clientlibs/python/tests/test_client.py	Adds tests for request payload + decision-point caching behavior.
clientlibs/python/tests/test_api_service.py	Adds API-service test for sending site + default target.
clientlibs/python/src/upgrade_client_lib/types/requests.py	Adds optional site/target to AssignRequest model.
clientlibs/python/src/upgrade_client_lib/data_service.py	Adds assignment-key helper + upsert merge into cache.
clientlibs/python/src/upgrade_client_lib/client.py	Adds decision-point scoped fetching and cache merge logic.
clientlibs/python/src/upgrade_client_lib/api_service.py	Sends site/target only when decision-point scoped (defaulting target to "").
clientlibs/js/src/UpGradeClient/UpgradeClient.ts	Adds decision-point options to getAllExperimentConditions + merges into cache.
clientlibs/js/src/types/requests.ts	Adds optional site/target to assign request type.
clientlibs/js/src/DataService/DataService.ts	Adds upsert merge behavior for decision-point assignment caching.
clientlibs/js/src/DataService/DataService.spec.ts	Adds unit tests for new upsert merge behavior.
clientlibs/js/src/ApiService/ApiService.ts	Sends site/target only when site is provided (defaulting target to "").
clientlibs/js/src/ApiService/ApiService.spec.ts	Adds test verifying site + normalized target in request body.
clientlibs/java/src/main/java/org/upgradeplatform/requestbeans/ExperimentRequest.java	Extends request bean to include site/target.
clientlibs/java/src/main/java/org/upgradeplatform/client/ExperimentClient.java	Adds decision-point fetch with merge-upsert caching and ignore-cache option.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[bcb37]

This is possibly a concern. If we're creating a cache key for each context/decision point, there could potentially be many more cold misses. That might increase the number of assign calls made.

[danoswaltCL]

yeah. hard to know what that side-effect will be, if these calls are legitimately just really quick, it may not matter, but it would change things. bah why's it always gotta be complicated, maybe i need to back off my claim that this is a straightforward change. even if these lookups are quick and the traffic is steady enough that most dps are accounted for in the caches, i suppose the real new side-effect is that every decision-point lookup that can't find a decision-point key in the cache will still need to check the database because the absence of it in the cached data no longer means we can skip looking it up. whereas today we can treat the context-based cache as authoritative and if it's not there, we can assume no assignment and skip the lookup. maybe this is the realization you came to also. sigh. this may not be the silver bullet i hoped it would be.

[bcb37]

One way we might be able to solve one problem without creating another is to either a) move to only decision-point cache keys (and then make sure all of the caches for all decision points are populated on a full fetch) OR b) stick with the current context-base cache keys and modify, rather than replace, the stored data on decision-point fetches.

[Copilot]

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (3)

packages/backend/src/api/services/ExperimentService.ts:283

• getCachedValidExperiments uses a cache key based only on context, but it can return either the full-context experiment set or a decision-point-scoped subset depending on whether site is provided. Because both variants share the same cache key, whichever call happens first can poison the cache for the other call pattern.

  public async getCachedValidExperiments(context: string, site: string, target: string): Promise<Experiment[]> {
    const cacheKey = CACHE_PREFIX.EXPERIMENT_KEY_PREFIX + context;
    const fetchByDecisionPoint = site !== undefined && site !== null;
    return this.cacheService
      .wrap(

packages/backend/src/api/services/ExperimentAssignmentService.ts:703

• site/target are typed as required string parameters in getExperimentsForUser, but the call site passes decisionPoint?.site / decisionPoint?.target, which can be undefined when no decision point is provided. The current types don’t reflect actual usage (and ExperimentService.getCachedValidExperiments also checks for undefined), making it easy to accidentally misuse these APIs.

    previewUser: PreviewUser,
    context: string,
    site: string,
    target: string
  ): Promise<Experiment[]> {

packages/backend/test/unit/controllers/ExperimentClientController.test.ts:293

• This test is named as if it verifies class-transformer normalization of a missing target, but it builds validator instances via Object.assign(...), which does not run @Transform. As written, decisionPoint.target stays undefined and the assertion only checks that the controller forwards the object, not that normalization happens.

      const decisionPoint = Object.assign(new DecisionPointValidator(), {
        site: 'CurriculumSequence',
      });

      const response = await controller.getAllExperimentConditions(

[Copilot]

Pull request overview

Copilot reviewed 25 out of 25 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (5)

packages/backend/src/api/services/ExperimentService.ts:307

• getCachedValidExperiments uses a cache key based only on context, but it can now return either full-context experiments or decision-point-scoped experiments depending on site. This can cause cache pollution (e.g., a DP-scoped fetch populates the context cache and later callers get the wrong experiment set, or vice versa). Consider bypassing the context cache for decision-point fetches (or include site/target in the cache key) and update the method signature so site/target are truly optional.

  public async getCachedValidExperiments(context: string, site: string, target: string): Promise<Experiment[]> {
    const cacheKey = CACHE_PREFIX.EXPERIMENT_KEY_PREFIX + context;
    const fetchByDecisionPoint = site !== undefined && site !== null;
    return this.cacheService
      .wrap(

packages/backend/test/integration/utils/index.ts:97

• getAllExperimentConditions expects decisionPoint to be omitted/undefined when not used. Passing null doesn’t match the method’s TS signature and will fail typechecking in stricter configs; it also doesn’t match the API shape (the validator treats a present-but-null decisionPoint as invalid). Use undefined here instead.

    null,

packages/backend/test/integration/UserNotDefined/index.ts:16

• getAllExperimentConditions expects decisionPoint to be omitted/undefined when not used. Passing null doesn’t match the method’s TS signature and will fail typechecking in stricter configs; it also doesn’t match the API shape (the validator treats a present-but-null decisionPoint as invalid). Use undefined here instead.

      null,

clientlibs/js/src/UpGradeClient/UpgradeClient.ts:373

• The docstring says this will not make a network call if getAllExperimentConditions() has already been called, but the implementation now triggers a decision-point-scoped fetch when that decision point isn’t in the cache. Update the docs to reflect that behavior.

  async getDecisionPointAssignment(site: string, target = ''): Promise<Assignment | null> {
    await this.getAllExperimentConditions({
      site,
      target,
    });

clientlibs/python/src/upgrade_client_lib/types/requests.py:37

• This adds a new DecisionPointRef, but this module already defines DecisionPointRef later (used by other request models). With from __future__ import annotations, that name collision can also change how earlier annotations are resolved at runtime. Rename this new type (or reuse the existing one) to avoid redefining DecisionPointRef in the same module.

class DecisionPointRef(BaseModel):
    site: str
    target: str | None = None


class AssignRequest(BaseModel):
    userId: str
    context: str
    decisionPoint: DecisionPointRef | None = None