Skip to content

Do not cast integer as string ids + Fix Zero ID + PHP Coduo #93

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
yoanm merged 6 commits into from
May 6, 2023
Merged

Do not cast integer as string ids + Fix Zero ID + PHP Coduo #93

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fbd0ad8
Do not cast integer as string ids
yoanm May 5, 2023
7149d10
Replace PHPUnit exact match with PHP-coduo
yoanm May 5, 2023
3b0fd12
Allow for zero IDs; notifications are only NULL
bradjones1 Jul 2, 2021
3b594ab
Fix https://github.com/yoanm/php-jsonrpc-server-sdk/issues/94
yoanm May 6, 2023
2955c9a
Remove useless empty lines
yoanm May 6, 2023
746ae8d
Fix lint
yoanm May 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
},
"require-dev": {
"behat/behat": "^3.9.0",
"coduo/php-matcher": "^6.0",
"dvdoug/behat-code-coverage": "^5.0",
"phpspec/prophecy": "^1.15",
"phpspec/prophecy-phpunit": "^2.0",
Expand Down
7 changes: 4 additions & 3 deletions features/bootstrap/FeatureContext.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
use Behat\Behat\Context\Environment\InitializedContextEnvironment;
use Behat\Behat\Hook\Scope\BeforeScenarioScope;
use Behat\Gherkin\Node\PyStringNode;
use DemoApp\Dispatcher\BehatRequestLifecycleDispatcher;
use Coduo\PHPMatcher\PHPUnit\PHPMatcherAssertions;
use PHPUnit\Framework\Assert;
use Tests\Functional\BehatContext\Helper\FakeEndpointCreator;

Expand All @@ -13,6 +13,8 @@
*/
class FeatureContext extends AbstractContext
{
use PHPMatcherAssertions;

const KEY_JSON_RPC = 'jsonrpc';
const KEY_ID = 'id';
const KEY_RESULT = 'result';
Expand Down Expand Up @@ -75,8 +77,7 @@ public function thenLastResponseShouldBeAValidJsonRpcError()
*/
public function thenIShouldHaveTheFollowingResponse(PyStringNode $expectedResult)
{
// Decode content to get rid of any indentation/spacing/... issues
Assert::assertEquals(
$this->assertMatchesPattern(
$this->jsonDecode($expectedResult->getRaw()),
$this->getLastResponseDecoded()
);
Expand Down
3 changes: 2 additions & 1 deletion src/App/Creator/ResponseCreator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ public function createEmptyResponse(JsonRpcRequest $fromRequest = null) : JsonRp
->setIsNotification($fromRequest->isNotification())
;

if ($fromRequest->getId()) {
// Notification response doesn't have any ID defined (notification request doesn't either)
if (!$fromRequest->isNotification()) {
$response->setId($fromRequest->getId());
}

Expand Down
6 changes: 1 addition & 5 deletions src/App/Serialization/JsonRpcRequestDenormalizer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,7 @@ protected function bindIdIfProvided(JsonRpcRequest $request, array $item) : void
{
/** If no id defined => request is a notification */
if (isset($item[self::KEY_ID])) {
$request->setId(
$item[self::KEY_ID] == (string)((int)$item[self::KEY_ID])
? (int)$item[self::KEY_ID] // Convert it in case it's a string containing an int
Copy link
Owner Author

@yoanm yoanm May 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure why I did that 🤔 Saw nothing on JSON-RPC specification regarding that point => removed

EDIT: Most likely a matter of typehint, but there is already a check on setId method to ensure id is either a string or an interger

: (string)$item[self::KEY_ID] // Convert to string in all other cases
);
$request->setId($item[self::KEY_ID]);
}
}

Expand Down
12 changes: 12 additions & 0 deletions tests/Functional/App/Creator/ResponseCreator/CreateResultResponseTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,4 +40,16 @@ public function testShouldBindResultAndRequestParamToResponse()
$this->assertSame($result, $response->getResult());
$this->assertFromRequestBinding($fromRequest, $response);
}

/**
* Bug fix: https://github.com/yoanm/php-jsonrpc-server-sdk/issues/94
*/
public function testShouldConvertRequestWithZeroIdToResponseWithZeroId()
{
$fromRequest = $this->createRequest(self::DEFAULT_METHOD, self::DEFAULT_JSONRPC, 0);

$response = $this->responseCreator->createEmptyResponse($fromRequest);

$this->assertSame(0, $response->getId());
}
}
29 changes: 0 additions & 29 deletions tests/Technical/App/Serialization/JsonRpcRequestDenormalizerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,35 +24,6 @@ protected function setUp(): void
$this->requestDenormalizer = new JsonRpcRequestDenormalizer();
}

/**
* @dataProvider integerRequestIdProvider
* @param mixed $requestId
*/
public function testDenormalizeShouldCastIdToIntWhenIdIs($requestId)
{
$item = [
'jsonrpc' => 'fake-json-rpc-version',
'method' => 'fake-method',
'id' => $requestId,
];

$result = $this->requestDenormalizer->denormalize($item);

$this->assertSame((int) $result->getId(), $result->getId());
}

public function integerRequestIdProvider()
{
return [
'real integer' => [
'requestId' => 321,
],
'integer stored as string' => [
'requestId' => '321',
],
];
}

/**
* @dataProvider invalidParamListProvider
*
Expand Down