feat(iv): add externalId and requiresJwt to Operation base class (2/6)

[nan-li]

Description

One Line Summary

PR 2 of 6 against the Identity Verification integration branch — mechanical refactor that adds externalId: String? and requiresJwt: Boolean to the Operation base class so the next PR can implement queue-level IV gating. No runtime behavior change.

Details

Motivation

The next PR in the series (PR 3/6, "queue-level IV") needs to read op.externalId polymorphically from OperationRepo. Today externalId is declared on LoginUserOperation and TrackCustomEventOperation only — OperationRepo can't read it from a generic Operation reference without casting to each of the 16 subclass types. This PR hoists the property (and a companion requiresJwt flag) to the base class so downstream consumers can read them uniformly.

Kept deliberately narrow so the base-class refactor has its own reviewable surface; the behavior changes (pre-HYDRATE deferral, FAIL_UNAUTHORIZED handling, IdentityVerificationService with anon-op purge) come in the next PR.

Scope

Operation base class:

• New var externalId: String? — captures the externalId of the user the op was enqueued for. Nullable because anonymous users have no externalId; base-class declaration keeps the type String? uniformly across all subclasses.
• New open val requiresJwt: Boolean get() = true — declares whether an op's backend endpoint needs a JWT when IV is active. Subclasses may override to false for endpoints that don't require auth (none do yet — default true covers every current op).

Operation subclasses (16):

• All 16 constructors gain externalId: String? as the 3rd parameter (after appId and onesignalId).
• LoginUserOperation and TrackCustomEventOperation shed their local externalId property in favor of the inherited base-class property (previously defined on each subclass independently).
• TransferSubscriptionOperation is the one exception in parameter position — its constructor is (appId, subscriptionId, onesignalId, externalId) to match the existing layout where onesignalId comes after subscriptionId.

Call sites updated to pass externalId at construction time (10 files):

• IdentityModelStoreListener, PropertiesModelStoreListener, SubscriptionModelStoreListener — read _identityModelStore.model.externalId.
• SessionListener (session start + end).
• TrackGooglePurchase (purchase tracking).
• UserSwitcher (legacy player ID migration).
• RebuildUserService (reconstructs queued ops from cached models).
• UserRefreshService (foreground user refresh).
• LoginUserOperationExecutor, LoginUserFromSubscriptionOperationExecutor, SubscriptionOperationExecutor (ops they emit as side effects, e.g. post-login RefreshUserOperation, post-subscription-recreate CreateSubscriptionOperation).

PropertiesModelStoreListener gains IdentityModelStore as a constructor dependency — the properties model doesn't carry externalId, so the listener has to read it from the identity model.

Placement decision: base class vs per-subclass

onesignalId is declared on each subclass individually, not on the base. Considered matching that convention for externalId/requiresJwt, but chose base-class placement:

• externalId is naturally nullable; String? works uniformly across every subclass without weakening any type.
• Cross-cutting consumers (next PR's hasValidJwtIfRequired, removeOperationsWithoutExternalId) need to read op.externalId polymorphically — that requires at least an abstract declaration on the base. Per-subclass with abstract-on-base would mean ~80 lines of boilerplate (override + getter/setter × 16 subclasses) vs the 5-line base-class declaration.
• requiresJwt has natural override semantics (open val with default), which is idiomatic on a base class.

name on Operation is precedent for a non-nullable universal property on the base, so there's no technical constraint keeping onesignalId off the base — that placement is a historical style preference that this PR does not touch.

What is NOT in this PR

• IdentityVerificationService — PR 3/6.
• OperationRepo IV gating (hasValidJwtIfRequired dispatch, FAIL_UNAUTHORIZED handler, pre-HYDRATE deferral in getNextOps) — PR 3/6.
• HTTP layer JWT attachment — PR 4/6.
• Public API (login(externalId, jwt), updateUserJwt, listeners) — PR 5/6.
• Logout + IAM integration — PR 6/6.

Testing

Unit testing

• Compile verified — every call site updated to pass externalId (see compile errors list narrowed from 50+ to 0).
• All existing test fixtures updated to the new constructor signature. Most were mechanical substitutions (add null as 3rd arg for anon/un-scoped tests; add the relevant externalId where the test exercises it).
• 791 core tests run, 789 pass, 2 fail — both are pre-existing SDKInitTests failures on the integration branch (unrelated: one is about an error-message assertion, the other about init not blocking). Same two failures exist on feat/iv-foundation-01 without this PR.

Manual testing

Not applicable — this PR adds a property + parameter with no runtime behavior change. Every call site still passes identically-shaped data; the only difference is that externalId is now available for polymorphic reads in the next PR.

Affected code checklist

• Notifications
• Outcomes
• Sessions (session listener passes externalId to track-session ops)
• In-App Messaging
• REST API requests (only op shape; no new network traffic)
• Public API changes

Checklist

Overview

• I have filled out all REQUIRED sections above
• PR does one thing — adds two properties to Operation and updates every subclass + call site
• Any Public API changes are explained in the PR details and conform to existing APIs (no public API changes)

Testing

• I have included test coverage for these changes (existing tests cover via construction; no new behavior to test in this PR)
• All automated tests pass locally (pre-existing SDKInitTests failures are unrelated — see Manual testing)
• Manual testing not applicable — no runtime behavior change

Final pass

• Code is as readable as possible.
• I have reviewed this PR myself, ensuring it meets each checklist item

[nan-li]

@claude review

[claude]

🟡 The hoisted externalId property on Operation (Operation.kt:24-28) drops the private set visibility that existed on both LoginUserOperation and TrackCustomEventOperation before this PR, so the setter is now public. Consider internal set to match the codebase precedent at LoginUserOperation.kt:42 where existingOnesignalId uses internal set specifically to preserve the "captured at construction" invariant documented in the KDoc.

Extended reasoning...

What changed

Before this PR, LoginUserOperation and TrackCustomEventOperation each declared their own externalId property with private set(value), making it read-only to external callers once constructed. Hoisting the property to the Operation base class at Operation.kt:24-28 dropped that visibility qualifier:

var externalId: String?
    get() = getOptStringProperty(::externalId.name)
    set(value) {                                   // <-- no visibility modifier = public
        setOptStringProperty(::externalId.name, value)
    }

Since every Operation subclass is public, any caller holding an Operation reference can now do op.externalId = "whatever" after the op has been enqueued, whereas previously that was a compile-time error for these two classes. This silently loosens the API surface across all 16 subclasses.

Why this contradicts the intended invariant

The new KDoc explicitly states: "Captured at construction time so the op stays bound to its original user even if the current user changes before the op executes." A public setter defeats that invariant — a caller that accidentally writes the field could re-bind the op to a different user between enqueue and execute.

Addressing the refutation

The refutation makes three points that are all factually correct but don't change the conclusion:

1. "Model.setOptStringProperty is public, so the invariant was never truly enforceable." True, a determined caller can always bypass typed property visibility via the generic setOptStringProperty("externalId", ...) route. But private set still prevented accidental mutations through the typed Kotlin setter, which is the common failure mode. Removing it removes that tripwire.
2. "private set won't compile — 16 subclass constructors in other files do this.externalId = externalId." Agreed, which is exactly why the fix is internal set (not private set). internal permits the same-module subclass constructors while still blocking external module callers — and this is the precedent at LoginUserOperation.kt:42 (existingOnesignalId) with the comment // internal so OperationRepo can merge during de-dupe.
3. "These are internal classes; no app-developer consumer today." True today, but this PR is explicitly positioned as scaffolding for PR 3/6's OperationRepo IV gating. As that consumer lands, keeping the visibility tight now prevents future mutations from slipping in unnoticed.

Proof walkthrough

Before this PR:

val op: Operation = LoginUserOperation("app", "osid", "user-A", null)
op.externalId = "user-B"   // compile error: cannot assign to 'externalId': the setter is private in LoginUserOperation

After this PR:

val op: Operation = LoginUserOperation("app", "osid", "user-A", null)
op.externalId = "user-B"   // compiles cleanly, silently rebinds the op

Fix

One-token change at Operation.kt:24-28:

var externalId: String?
    get() = getOptStringProperty(::externalId.name)
    internal set(value) { //  so same-module subclass constructors can assign
        setOptStringProperty(::externalId.name, value)
    }

Severity: nit

All three confirming verifiers independently landed on nit, and that's the right call: (1) no current caller actually mutates op.externalId post-construction, so there's no observable runtime regression; (2) Model.setOptStringProperty remains a public escape hatch, so the fix hardens the common path but doesn't guarantee the invariant; (3) one-line fix, purely API-hygiene.

[claude]

🟣 Pre-existing: the KDoc block at lines 30-32 reads "The optional external ID of this newly logged-in user. Must be unique for the [appId]." but is attached to the subscriptionId property, not an externalId one. This PR doesn't introduce the bug, but by hoisting externalId to the Operation base class it makes the misplaced doc actively misleading — readers of this file now see a description of "external ID" above subscriptionId while the real externalId is silently inherited. Worth fixing the comment in this PR while the file is already being touched (swap in the correct description for subscriptionId).

Extended reasoning...

What the bug is

In LoginUserFromSubscriptionOperation.kt, lines 30-32 contain a KDoc block:

/**
 * The optional external ID of this newly logged-in user. Must be unique for the [appId].
 */
var subscriptionId: String

The description clearly belongs to an externalId field (it even says "external ID" explicitly), but it is attached to the subscriptionId property. This is a copy-paste artifact — the exact same KDoc string used to sit on LoginUserOperation.externalId (see the deleted block in this PR's diff of LoginUserOperation.kt).

How it manifests

Any IDE or documentation tool that renders KDoc will show "The optional external ID of this newly logged-in user. Must be unique for the [appId]." when the user hovers over or looks up LoginUserFromSubscriptionOperation.subscriptionId. That is factually wrong — subscriptionId is the legacy player ID being migrated from, not an external ID.

Why this PR makes it worse

Before this PR, the stale comment was confusing but self-contained: a reader looking at LoginUserFromSubscriptionOperation would see a wrong description on subscriptionId with no nearby externalId to explain where the comment came from.

After this PR, externalId is hoisted to the Operation base class, so LoginUserFromSubscriptionOperation does now have an inherited externalId property. A reader sees:

• The new constructor at line 45 takes and sets an externalId.
• The class doesn't declare externalId locally (it's inherited silently from Operation).
• A KDoc describing "the optional external ID ... Must be unique for the appId" sits immediately above subscriptionId at line 33.

The natural reading is that this doc block is describing the inherited externalId — but it is syntactically attached to subscriptionId, so IDE tooling will continue to associate the wrong doc with subscriptionId. That divergence between "what a human reads" and "what the doc actually documents" is strictly worse than the pre-PR state.

Step-by-step proof

1. Check out this PR branch.
2. Open OneSignalSDK/onesignal/core/src/main/java/com/onesignal/user/internal/operations/LoginUserFromSubscriptionOperation.kt.
3. Observe lines 30-37:

   /**
    * The optional external ID of this newly logged-in user. Must be unique for the [appId].
    */
   var subscriptionId: String
       get() = getStringProperty(::subscriptionId.name)
       private set(value) {
           setStringProperty(::subscriptionId.name, value)
       }

4. Compare with the deleted block in LoginUserOperation.kt (lines 35-43 of the pre-PR file):

   /**
    * The optional external ID of this newly logged-in user. Must be unique for the [appId].
    */
   var externalId: String?

   The text is identical — the comment was cloned onto subscriptionId at some point and never fixed.
5. Hover subscriptionId in any Kotlin-aware IDE: the rendered KDoc will read "The optional external ID of this newly logged-in user. Must be unique for the [appId]." — describing the wrong field.

Impact

Documentation only — no runtime effect, no API effect. But this file is the one place in the PR where a misplaced externalId description sits next to an inherited-and-invisible externalId property, so it is the highest-signal cleanup opportunity in the diff.

Fix

Replace the KDoc with an accurate description of subscriptionId, e.g.:

/**
 * The subscription ID used to look up the user to log in as. Typically the legacy v4 player ID
 * being migrated from.
 */
var subscriptionId: String

One-line fix; no behavior change.