Data Sync - #138
Data Sync#138jakub-grzesiowski wants to merge 9 commits into
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
| * Two clients are involved: an admin signing its requests with the secret key, which grants the | ||
| * tokens and owns the fixtures, and a token-only client that has no credentials beyond the token | ||
| * under test. That split is what makes the denials meaningful - a client carrying both a signature | ||
| * and a token is rejected outright, so the token is the only thing granting the access asserted |
There was a problem hiding this comment.
What does it mean "a client carrying both a signature
- and a token is rejected outright" ?
Is this server rejecting such request?
There was a problem hiding this comment.
Yes DataSync will reject a request if it has both auth and signature
| foreach ($this->sort as $key => $value) { | ||
| if (is_int($key)) { | ||
| $entries[] = $value; | ||
| } elseif ($value === 'asc' || $value === 'desc') { |
There was a problem hiding this comment.
Is this possible that $value is 'DESC' ?
If so then it seem that else branch will be executed causing ascending sort?
| if (in_array($typeName, self::ENTITY_TYPES, true)) { | ||
| $entity = PNDataSyncEntity::fromPayload(array_merge($data, [ | ||
| 'entityClass' => $className, | ||
| 'entityClassVersion' => $classVersion, |
There was a problem hiding this comment.
Shouldn't we include also:
'entityClassLevel' => $classLevel
?
| } else { | ||
| $messageError = null; | ||
| try { | ||
| $extractedMessage = $this->processMessage($message->getPayload()); |
There was a problem hiding this comment.
There might be a problem when Crypto is configured processMessage() calls decrypt() that takes only string and objects not array DataSync event payload is an array causing TypeError
| protected const PATCH_MEDIA_TYPE = "application/json-patch+json"; | ||
|
|
||
| /** A delete answers 200 with no body. */ | ||
| protected const RESPONSE_MAY_BE_EMPTY = true; |
There was a problem hiding this comment.
Should't this apply only to Deleted operations that could return empty body. Other operations should not delete empty body?
| /** | ||
| * Response of the paginated membership list operation. | ||
| */ | ||
| class PNDataSyncMembershipsResult |
There was a problem hiding this comment.
This class seems very similar to:
PNDataSyncEntitiesResult
PNDataSyncRelationshipsResult
PNDataSyncUsersResult
PNDataSyncChannelsResult
Did you consider creation of base class that those classes can extend. Is such approach we could avoid code duplication?
| /** | ||
| * Response of the single-entity DataSync operations (create, get, set and update). | ||
| */ | ||
| class PNDataSyncEntityResult |
There was a problem hiding this comment.
Seem that this class is very similar to:
PNDataSyncRelationshipResult
PNDataSyncMembershipResult
PNDataSyncUserResult
PNDataSyncChannelResult
Did you consider creating baseClass to avoid duplication?
| * @param array<string, array<string, array<string, string>>> $projections | ||
| * @return $this | ||
| */ | ||
| public function dataSyncProjections($projections) |
There was a problem hiding this comment.
Seems that typo like "resource" instead of "resources" produces token with no projections and default view. Same with "pattern" vs "patterns".
Maybe it would make sense to add validation so such typos are catched and informed?
3 participants
No description provided.