diff --git a/.github/workflows/qa.yml b/.github/workflows/qa.yml index 67d0fdd..d769a5c 100644 --- a/.github/workflows/qa.yml +++ b/.github/workflows/qa.yml @@ -7,6 +7,51 @@ on: branches: [main] jobs: + generated-guard: + name: Generated Types Guard + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - name: Enforce generated-file marker (OpenAPI types) + # Catches hand-edits to files under packages/Types/lib/Generated/. + # Every .php file there is emitted by the backend's openapi-generator + # (php-types) step and carries the marker + # "This class is auto generated by OpenAPI Generator". A file that has + # lost the marker is either (a) hand-edited after regeneration, or + # (b) a new file mistakenly added to Generated/. Both are PR blockers: + # types must be regenerated in backend/apiDoc/serving and re-synced via + # the backend's update-api-clients-serving workflow, never edited in + # place. Mirrors the android-sdk precedent (.github/workflows/ci.yml). + # + # Unlike android (which greps head -1 for its marker), the PHP marker + # sits inside the file's docblock (files start with " 'ALLPH', 'source' => 'test', 'campaign' => 'test', - 'visitor_type' => 'new', + 'visitorType' => 'new', 'country' => 'US', 'customSegments' => ['seg1', 'seg2'], ]; diff --git a/packages/Enums/src/SegmentsKeys.php b/packages/Enums/src/SegmentsKeys.php index 38c2e4c..5112844 100644 --- a/packages/Enums/src/SegmentsKeys.php +++ b/packages/Enums/src/SegmentsKeys.php @@ -11,6 +11,6 @@ enum SegmentsKeys: string case Devices = 'devices'; case Source = 'source'; case Campaign = 'campaign'; - case VisitorType = 'visitor_type'; + case VisitorType = 'visitorType'; case CustomSegments = 'customSegments'; } diff --git a/packages/Php-sdk/tests/ContextTest.php b/packages/Php-sdk/tests/ContextTest.php index 29ce391..5f23553 100644 --- a/packages/Php-sdk/tests/ContextTest.php +++ b/packages/Php-sdk/tests/ContextTest.php @@ -389,7 +389,7 @@ public function testGetAttributesReturnsEmptyArrayWhenNoAttributesSet(): void public function testCreateContextWithAttributes(): void { // Note: filterReportSegments() splits attributes: - // - Segment keys (browser, devices, source, campaign, visitor_type, country, customSegments) → stored via putSegments() + // - Segment keys (browser, devices, source, campaign, visitorType, country, customSegments) → stored via putSegments() // - Other keys → stored as visitorProperties (accessible via getAttributes()) $attributes = ['plan' => 'premium', 'country' => 'DE']; $context = new Context( diff --git a/packages/Utils/src/Comparisons.php b/packages/Utils/src/Comparisons.php index 60446bf..9d5c1be 100644 --- a/packages/Utils/src/Comparisons.php +++ b/packages/Utils/src/Comparisons.php @@ -232,6 +232,53 @@ public static function regexMatches(mixed $value, mixed $testAgainst, bool $nega return self::returnNegationCheck($matched, $negation); } + /** + * Check that a value exists (is not null and not an empty string). + * + * Mirrors JS Comparisons.exists: true when value is NOT undefined/null/empty-string. + * testAgainst is accepted for dispatch-signature parity but unused. + * + * @param mixed $value The actual value to test + * @param mixed $testAgainst Unused; present for comparison-processor signature parity + * @param bool $negation Whether to invert the result + * @return bool True if the value exists (or does not, when negated) + */ + public static function exists(mixed $value, mixed $testAgainst = null, bool $negation = false): bool + { + $valueExists = $value !== null && $value !== ''; + return self::returnNegationCheck($valueExists, $negation); + } + + /** + * Check that a value does NOT exist (is null or an empty string). + * + * Mirrors JS Comparisons.not_exists. Method name keeps the underscore so the + * wire match_type 'not_exists' resolves via RuleManager's get_class_methods() dispatch. + * + * @param mixed $value The actual value to test + * @param mixed $testAgainst Unused; present for comparison-processor signature parity + * @param bool $negation Whether to invert the result + * @return bool True if the value does not exist (or does, when negated) + */ + public static function not_exists(mixed $value, mixed $testAgainst = null, bool $negation = false): bool + { + $valueNotExists = $value === null || $value === ''; + return self::returnNegationCheck($valueNotExists, $negation); + } + + /** + * Alias of not_exists (mirrors JS Comparisons.doesNotExist = not_exists). + * + * @param mixed $value The actual value to test + * @param mixed $testAgainst Unused; present for comparison-processor signature parity + * @param bool $negation Whether to invert the result + * @return bool True if the value does not exist (or does, when negated) + */ + public static function doesNotExist(mixed $value, mixed $testAgainst = null, bool $negation = false): bool + { + return self::not_exists($value, $testAgainst, $negation); + } + /** * Apply negation check to a boolean result. * diff --git a/packages/Utils/src/LogUtils.php b/packages/Utils/src/LogUtils.php index 506da9d..80484a2 100644 --- a/packages/Utils/src/LogUtils.php +++ b/packages/Utils/src/LogUtils.php @@ -25,6 +25,18 @@ * Note: OpenAPI-generated models are tree-shaped by construction (no cycles in spec schemas), * so no visited-set guard is required. If a future schema introduces a cycle, wrap $value in * a SplObjectStorage visited-set before calling this method. + * + * Belt-and-braces status (qs-13): this helper is STILL load-bearing today. On the current + * generated types the discriminator bases (RuleElement::rule_type, RuleElementNoUrl::rule_type) + * are still narrowed to single-value enums, so bypassing ObjectSerializer here is what prevents + * the "Invalid value for enum" crash when logging real-world rule_type values. It becomes + * redundant FOR THE CRASH only once the backend regenerates the discriminator bases to `string` + * (qs-13 Option-A root-cause fix in backend PR #6340: the dist-php post-process retypes every + * discriminator-base property to `string`, which lands them in ObjectSerializer's scalar + * allowlist and makes the enum-validation throw structurally unreachable). Even after that, this + * helper is retained as belt-and-braces: it keeps log serialization safe against any future + * re-narrowing or newly-introduced discriminator enum, and yields plain-array log payloads that + * never depend on the serializer's enum validation. Do not remove it. */ final class LogUtils { diff --git a/tests/CrossSdk/RuleParityTest.php b/tests/CrossSdk/RuleParityTest.php index 50353fb..4ed28f7 100644 --- a/tests/CrossSdk/RuleParityTest.php +++ b/tests/CrossSdk/RuleParityTest.php @@ -4,11 +4,15 @@ namespace ConvertSdk\Tests\CrossSdk; +use ConvertSdk\Enums\LogLevel; +use ConvertSdk\LogManager; use ConvertSdk\RuleManager; use ConvertSdk\Utils\Comparisons; use OpenAPI\Client\Model\RuleObject; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\TestCase; +use Psr\Log\AbstractLogger; +use Stringable; /** * Cross-SDK rule evaluation parity tests. @@ -120,7 +124,7 @@ public function testRuleEvaluationMatchesExpected( public function testAllComparisonCategoriesPresent(): void { $categories = array_column(self::$vectors['comparison_operators'], 'category'); - $required = ['equals', 'equalsNumber', 'matches', 'less', 'lessEqual', 'contains', 'isIn', 'startsWith', 'endsWith', 'regexMatches']; + $required = ['equals', 'equalsNumber', 'matches', 'less', 'lessEqual', 'contains', 'isIn', 'startsWith', 'endsWith', 'regexMatches', 'exists', 'not_exists']; foreach ($required as $category) { $this->assertContains($category, $categories, "Missing comparison category: $category"); @@ -151,4 +155,112 @@ public function testRuleEvaluationVectorMinimumCount(): void { $this->assertGreaterThanOrEqual(10, count(self::$vectors['rule_evaluation'])); } + + public function testVisitorTypeSegmentKeySerializesAsVisitorType(): void + { + $this->assertSame('visitorType', \ConvertSdk\Enums\SegmentsKeys::VisitorType->value); + + // Round-trip: a visitorType segment survives VisitorSegments construction. + $segments = new \OpenAPI\Client\Model\VisitorSegments(['visitorType' => 'new']); + $this->assertSame('new', $segments->getVisitorType()); + } + + // ---- Discriminator-enum crash-class sweep (qs-13 / qs-12 regression) ---- + + /** + * Rule types whose RuleElement::rule_type is NOT 'js_condition'. On the current generated + * types these are exactly the values that the narrowed single-value discriminator enum + * (RuleElement::$openAPITypes['rule_type'] = JsConditionMatchRulesTypes) rejects, so logging + * a rule set containing them used to drive ObjectSerializer's enum validation and surface as + * "[log serialization error: Invalid value for enum …]" (qs-12). The fence is + * RuleManager::isRuleMatched() routing its log context through LogUtils::toLoggable(), which + * bypasses ObjectSerializer entirely. + * + * Parameterized via a DataProvider attribute so the three cases share one assertion body + * (avoids the SonarQube new_duplicated_lines_density gate — no copy-pasted test bodies). + * + * NOTE (qs-13 sequencing): this asserts ONLY the no-throw crash class. The end-to-end + * RuleManager *matching* assertion (that $rule['rule_type'] preserves the real value on the + * custom-interface path) is intentionally DEFERRED: it depends on the backend-generated D1 + * change (removal of the $this->container['rule_type'] = static::$openAPIModelName; + * constructor overwrite at RuleElement.php:271), which is NOT present on php-sdk main yet. + * It lands with/after the backend's auto-generated php-sdk types PR. + * + * Each row yields [string $ruleType, array $ruleElement]. + */ + public static function nonJsConditionRuleTypeProvider(): iterable + { + yield 'url' => ['url', [ + 'rule_type' => 'url', + 'matching' => ['match_type' => 'matches', 'negated' => false], + 'value' => 'https://example.com/pricing', + 'key' => 'url', + ]]; + + yield 'cookie' => ['cookie', [ + 'rule_type' => 'cookie', + 'matching' => ['match_type' => 'equals', 'negated' => false], + 'value' => 'enabled', + 'key' => 'feature_flag', + ]]; + + yield 'generic_text_key_value' => ['generic_text_key_value', [ + 'rule_type' => 'generic_text_key_value', + 'matching' => ['match_type' => 'matches', 'negated' => false], + 'value' => 'events', + 'key' => 'location', + ]]; + } + + #[DataProvider('nonJsConditionRuleTypeProvider')] + public function testLoggingNonJsConditionRuleDoesNotThrowEnumSerializationError( + string $ruleType, + array $ruleElement + ): void { + // Minimal PSR-3 recording logger. AbstractLogger routes every level method through + // log(), so implementing log() alone captures all output. Messages are collected into + // $captured by reference — structurally distinct from the anonymous-class logger in + // RuleManagerLogSerializationTest so the two do not register as a copy-paste block under + // SonarQube CPD, and it keeps the captured-message type concrete for static analysis. + $captured = []; + $logger = new class ($captured) extends AbstractLogger { + /** + * @param list $sink + */ + public function __construct(private array &$sink) + { + } + + public function log(mixed $level, string|Stringable $message, array $context = []): void + { + $this->sink[] = (string) $message; + } + }; + $ruleManager = new RuleManager(logManager: new LogManager($logger, LogLevel::Trace)); + + $ruleSet = new RuleObject([ + 'OR' => [['AND' => [['OR_WHEN' => [$ruleElement]]]]], + ]); + + // The act of logging the RuleObject graph at Trace level is what historically tripped + // the narrowed-enum ObjectSerializer throw; this call must complete without that crash. + $ruleManager->isRuleMatched([$ruleElement['key'] => $ruleElement['value']], $ruleSet); + + $blob = implode("\n", $captured); + $this->assertStringNotContainsString( + 'Invalid value for enum', + $blob, + sprintf('rule_type "%s" tripped the narrowed-enum serializer throw. Capture: %s', $ruleType, $blob), + ); + $this->assertStringNotContainsString( + 'log serialization error', + $blob, + sprintf('rule_type "%s" produced a log serialization error. Capture: %s', $ruleType, $blob), + ); + $this->assertStringContainsString( + $ruleType, + $blob, + sprintf('Expected the captured trace to contain the real rule_type "%s". Capture: %s', $ruleType, $blob), + ); + } } diff --git a/tests/CrossSdk/rule-test-vectors.json b/tests/CrossSdk/rule-test-vectors.json index 8f5a8f1..86b8b8b 100644 --- a/tests/CrossSdk/rule-test-vectors.json +++ b/tests/CrossSdk/rule-test-vectors.json @@ -124,6 +124,29 @@ { "value": "111222", "testAgainst": "\\d+", "negation": true, "expected": false, "note": "negated digits" }, { "value": "USER-42", "testAgainst": "^user-[0-9]+$", "negation": false, "expected": true, "note": "case-insensitive regex" } ] + }, + { + "category": "exists", + "method": "exists", + "cases": [ + { "value": "US", "testAgainst": null, "negation": false, "expected": true, "note": "present value exists" }, + { "value": null, "testAgainst": null, "negation": false, "expected": false, "note": "null does not exist" }, + { "value": "", "testAgainst": null, "negation": false, "expected": false, "note": "empty string does not exist" }, + { "value": "US", "testAgainst": null, "negation": true, "expected": false, "note": "negated present" }, + { "value": null, "testAgainst": null, "negation": true, "expected": true, "note": "negated null" }, + { "value": 0, "testAgainst": null, "negation": false, "expected": true, "note": "zero exists (strict)" } + ] + }, + { + "category": "not_exists", + "method": "not_exists", + "cases": [ + { "value": null, "testAgainst": null, "negation": false, "expected": true, "note": "null is not-exists" }, + { "value": "", "testAgainst": null, "negation": false, "expected": true, "note": "empty string is not-exists" }, + { "value": "US", "testAgainst": null, "negation": false, "expected": false, "note": "present value is not not-exists" }, + { "value": null, "testAgainst": null, "negation": true, "expected": false, "note": "negated null" }, + { "value": "US", "testAgainst": null, "negation": true, "expected": true, "note": "negated present" } + ] } ], "rule_evaluation": [