fix: support both snake_case and camelCase versions of impression data #227

gastonfournier · 2024-09-24T14:59:10Z

Description

Impression data should have always been camelCase in unleash-edge. It was not by accident, and the php sdk is using the snake_case version which prevents users from upgrading to newer versions of edge where impression_data was changed to camelCase.

Fixes #226

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Unit tests
Spec Tests
Integration tests / Manual Tests

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

src/DTO/DefaultProxyFeature.php

src/Repository/DefaultUnleashProxyRepository.php

tests/DTO/DefaultProxyFeatureTest.php

gastonfournier · 2024-09-25T15:26:05Z

tests/Bootstrap/FileBootstrapProviderTest.php

@@ -75,7 +75,7 @@ public function testUnreadableFile()
        $file = $this->createTemporaryFile();
        chmod($file, 0222);
        $instance = new FileBootstrapProvider($file);
-        $this->expectException(RuntimeException::class);
+        $this->expectException(JsonException::class);


@RikudouSage I couldn't check your comments as I'm struggling with this test that fails locally. I'm building this Dockerfile (docker build . -t php-with-composer) to reduce noise and make it reproducible:

FROM php:8.3.10-fpm # Install system dependencies and clean cache RUN apt-get update && apt-get install -y \ git \ curl \ libpng-dev \ libonig-dev \ libxml2-dev \ libcurl4-openssl-dev libssl-dev \ zip \ unzip # Install PHP extensions RUN docker-php-ext-install pdo_mysql mbstring exif pcntl bcmath gd # Copy Composer from the official Composer image COPY --from=composer:latest /usr/bin/composer /usr/bin/composer COPY . /usr/src/unleash-client-php WORKDIR /usr/src/unleash-client-php RUN rm -f composer.lock RUN composer install CMD ["sh"]

And running the tests as: docker run -it --rm --name my-running-script php-with-composer composer phpunit and the test fails with the following error:

$ docker run -it --rm --name my-running-script php-with-composer composer phpunit > phpunit PHPUnit 9.6.21 by Sebastian Bergmann and contributors. Runtime: PHP 8.3.10 Configuration: /usr/src/unleash-client-php/phpunit.xml .........F..................................................... 63 / 233 ( 27%) ............................................................... 126 / 233 ( 54%) ............................................................... 189 / 233 ( 81%) ............................................ 233 / 233 (100%) Time: 00:25.133, Memory: 20.00 MB There was 1 failure: 1) Unleash\Client\Tests\Bootstrap\FileBootstrapProviderTest::testUnreadableFile Failed asserting that exception of type "JsonException" matches expected exception "RuntimeException". Message was: "Syntax error" at /usr/src/unleash-client-php/src/Bootstrap/FileBootstrapProvider.php:46 /usr/src/unleash-client-php/tests/Bootstrap/FileBootstrapProviderTest.php:79 . FAILURES! Tests: 233, Assertions: 895, Failures: 1. Script phpunit handling the phpunit event returned with error code 1

Do you know what can it be?

CI does prefer RuntimeException...

I don't :/ But JsonException generally means that there was a syntax error in some JSON, any chance you're providing a non-JSON string somewhere where JSON is expected?

Ah, figured it out, I assume you run the container under its default user, root? The test creates a file and makes it non-readable using chmod, but root has access to everything regardless of read/write permissions, thus the file is readable but because it's empty it fails with a JsonException.

If I run it as the current user, it works:

docker run --rm -it -u $(id -u):$(id -g) php-with-composer vendor/bin/phpunit

gastonfournier · 2024-09-25T16:29:03Z

src/DTO/DefaultProxyFeature.php

+     *     impression_data?: bool,
+     *     impressionData?: bool


One will be set, the other won't, we don't know which one because it depends on the version of the backend

gastonfournier · 2024-09-25T16:30:02Z

src/DTO/DefaultProxyFeature.php

@@ -88,6 +92,7 @@ public function jsonSerialize(): array
            'enabled' => $this->enabled,
            'variant' => $this->variant,
            'impression_data' => $this->impressionData,
+            'impressionData' => $this->impressionData, // maybe we don't need this


I'm unsure what are we using this serialization for... maybe caching?

gastonfournier · 2024-09-25T16:30:12Z

src/Repository/DefaultUnleashProxyRepository.php

+     *     impression_data?: bool,
+     *     impressionData?: bool


One will be set, the other won't, we don't know which one because it depends on the version of the backend

src/Repository/DefaultUnleashProxyRepository.php

gastonfournier · 2024-09-26T14:28:56Z

src/DTO/DefaultProxyFeature.php

@@ -87,7 +90,8 @@ public function jsonSerialize(): array
            'name' => $this->name,
            'enabled' => $this->enabled,
            'variant' => $this->variant,
-            'impression_data' => $this->impressionData,
+            'impression_data' => $this->impressionData, // deprecated


We output both formats because this is public and might be in use.

sighphyre

LGTM

RikudouSage

Looks good!

gastonfournier · 2024-10-02T08:53:05Z

Commit was merged in main, Github had some issue with this one

fix: support both snake_case and camelCase versions of impression data

6d5fb9e

RikudouSage reviewed Sep 24, 2024

View reviewed changes

gastonfournier added 3 commits September 25, 2024 11:30

WIP

b586071

Fix test expectations

ddc0ab6

Revert unintended change

e9dbca5

gastonfournier commented Sep 25, 2024

View reviewed changes

gastonfournier added 5 commits September 25, 2024 17:54

Accept both snake_case and camelCase

584b21c

code format

cac078e

Impression data might be not set

0218705

Add missing comma

14741d9

Handle impression data

c9568bc

gastonfournier commented Sep 25, 2024

View reviewed changes

gastonfournier added 3 commits September 26, 2024 15:57

Comments

7b58eb5

Change code to align with static analysis

14dd119

test: verify backward compatibility

07b6c4f

gastonfournier commented Sep 26, 2024

View reviewed changes

gastonfournier requested a review from RikudouSage September 26, 2024 14:30

gastonfournier self-assigned this Sep 26, 2024

gastonfournier requested a review from sighphyre September 26, 2024 14:30

tests: add full blown test for snake case impression data

ce70ac4

sighphyre approved these changes Oct 1, 2024

View reviewed changes

RikudouSage approved these changes Oct 1, 2024

View reviewed changes

pallimalilsuhail mentioned this pull request Oct 1, 2024

Bug: validateResponse fails on impression_data #226

Closed

1 task

gastonfournier closed this Oct 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: support both snake_case and camelCase versions of impression data #227

fix: support both snake_case and camelCase versions of impression data #227

gastonfournier commented Sep 24, 2024 •

edited

Loading

gastonfournier Sep 25, 2024

gastonfournier Sep 25, 2024

RikudouSage Oct 1, 2024

RikudouSage Oct 1, 2024

gastonfournier Sep 25, 2024

gastonfournier Sep 25, 2024

gastonfournier Sep 25, 2024

gastonfournier Sep 26, 2024

sighphyre left a comment

RikudouSage left a comment

gastonfournier commented Oct 2, 2024

fix: support both snake_case and camelCase versions of impression data #227

fix: support both snake_case and camelCase versions of impression data #227

Conversation

gastonfournier commented Sep 24, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sighphyre left a comment

Choose a reason for hiding this comment

RikudouSage left a comment

Choose a reason for hiding this comment

gastonfournier commented Oct 2, 2024

gastonfournier commented Sep 24, 2024 •

edited

Loading