-
Notifications
You must be signed in to change notification settings - Fork 33
feat(capabilities): add capabilities functionality to the SDK #551
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.
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #551 +/- ##
============================================
+ Coverage 56.29% 56.54% +0.24%
- Complexity 1863 1988 +125
============================================
Files 118 125 +7
Lines 5437 5764 +327
============================================
+ Hits 3061 3259 +198
- Misses 2376 2505 +129 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/Services/CoreApi/Generated/Capabilities/lib/Model/RefTypesCarrierV2.php
Show resolved
Hide resolved
|
Please also ensure the commit message AND PR title conform to a conventional commit format and are suitable for inclusion into changelogs |
0c86d99 to
d29eda7
Compare
joerivanveen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed would be nice to exclude the generated directory from coverage reports, to keep the data understandable.
CONTRIBUTING.md
Outdated
| This command runs `swaggerapi/swagger-codegen-cli-v3` inside a Docker container, using the specification at: | ||
|
|
||
| ``` | ||
| https://api.myparcel.nl/openapi.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't have to be in this PR right now, but we should have a ticket for and a means to generate a client for the acceptance API and possibly testing as well. Ideally in a way that does not require a completely new generation of a new client (e.g. being able to switch at runtime to a pregenerated acceptance client?)
Does the swagger codegen offer any solutions for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FreekVR If the acceptance spec would be the same as prod. we'll keep one generated client and switch runtime via an env flag in the factory (using Configuration::setHost()). Swagge also supports server switching when multiple servers are defined in the spec, but our Core API only defines one. If the specs differ, we can generate a second client under a different namespace and let the factory pick the correct one based on the environment flag.
I'll write a ticket if you want me to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to add a ticket -- any new or changed fields would yield a new schema, and the only way to prepare our client code would be to generate a new api client as well. Perhaps we could simply solve this with a branching strategy, but would be good to create ticket to think about this and at the very least document that approach there.
9c87a6d to
b8ac8b6
Compare
e65e24d to
807bf50
Compare
ad5ed2a to
56bedfe
Compare
56bedfe to
41bffa7
Compare
67c9fde to
cb43c00
Compare
cb43c00 to
4deb8d4
Compare
2d3eded to
4dd5073
Compare
37fe840 to
2eb3d36
Compare
Implements a Capabilities API client to enable dynamic discovery of carrier-specific functionality, preparing the SDK architecture for future configuration refactoring. Phase 1: - Generated client from Core API spec (Located in src/Services/CoreApi/Generated/Capabilities/). - Integration layer: HttpCapabilitiesClient, CapabilitiesMapper (SDK <-> Core API transformation), CapabilitiesClientFactory. - Service Facade: CapabilitiesService & CapabilitiesServiceInterface - Request/Response models: CapabilitiesRequest (Immutable request builder) & CapabilitiesResponse with Built-in input normalization for flexibility. - Type safety 'Enums': Carrier, DeliveryType, Direction, PackageType, TransactionType. All enums include normalize() methods for flexible input handling. - Test coverage: Unit tests: Service pass-through behaviour, Integration tests: Request/response mapping logic and Live Smoke Tests: Real API validation (auto skipped without API keys). Follow-up work for Phase 2: - Add missing Core API fields: The following fields are part of the Core Capabilities v2 spec but are intentionally postponed for clarity and maintainability. They introduce structural complexity that will be handled in the next iteration. • sender (simple object mapping) • physicalProperties (requires value/unit data classes) • options (dynamic shipment options map) - Refactor static SDK configuration to fetch capabilities dynamically - Add caching, validation, and developer documentation INT-1054
…with generated enums and all API fields - Replaced Model/Capabilities/Enum/* with generated RefTypes* models - Added sender, options, physicalProperties to CapabilitiesRequest and CapabilitiesMapper - Added tests for new functionality - Removed TODO markers - implementation now matches Core API v2 spec completely - Added OpenAPI generator dev dependency and composer script for API updates INT-1054
- Switch from Swagger Codegen to OpenAPI Generator (supports OpenAPI 3.1) - Rename namespace from Capabilities to Shipments - Create scripts/fix-generated-code.sh for PHP 7.4 ArrayAccess compatibility - Integrate post-processing into composer generate:api command - Update CONTRIBUTING.md documentation
- Remove phpLegacySupport option (not valid) - Add fix-generated-php74.sh script to remove PHP 8.0+ type hints - Fix all generated models to be PHP 7.4 compatible
2eb3d36 to
ae3575c
Compare
- Add openapi-generator-mapper.yml with mappings for Capabilities V2 models - Update CapabilitiesMapper to use shorter class names - Maps verbose names like CapabilitiesPostCapabilitiesRequestV2PhysicalPropertiesHeight to PhysicalPropertiesHeight
- Generated models now use shorter, cleaner names - CapabilitiesRecipient, CapabilitiesSender, etc. instead of verbose V2 names - PhysicalPropertiesHeight/Length/Width/Weight instead of full nested names
- Remove mixed type hints not supported in PHP 7.4 - Applied via scripts/fix-generated-php74.sh
- Add fix-generated-php74 step to composer generate:api - Ensures generated code is always PHP 7.4 compatible
- Remove docker fix from composer script (script not in container) - Keep fix step in workflow where it runs directly on runner - Mappings still apply via docker-compose config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think its intentional to remove all the .iea files. Please restore these as long as we haven't replaced them with alternative solutions.
| with: | ||
| fetch-depth: 0 | ||
|
|
||
| - name: Set up PHP + Composer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest using myparcelnl/php-xd:7.4 which is our own image with the correct PHP version we want to support and comes with composer
| - name: Generate API client | ||
| run: composer generate:api | ||
|
|
||
| - name: Fix generated code for PHP 7.4 compatibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use a PHP 7.4 image above, it should result in a version of the generator being installed which outputs compatible code - and that would make this step redundant.
| * | ||
| * @see \MyParcelNL\Sdk\Mapper\CapabilitiesMapper | ||
| */ | ||
| class CapabilitiesRequest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly is the advantage of using this over ex. CapabilitiesPostCapabilitiesRequestV2 directly in code consuming our SDK?
| $options = new CoreOptionsV2(); | ||
|
|
||
| // Map all available options - empty values mean "enabled" | ||
| foreach ($optionsData as $key => $value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This mapping would need updating for each new or changed option. Also not a fan of the keys being typed as just strings here.
| $stack->push(Middleware::mapRequest(function (\Psr\Http\Message\RequestInterface $request) { | ||
| $path = (string) $request->getUri()->getPath(); | ||
| if (false !== strpos($path, self::CAPABILITIES_PATH)) { | ||
| return $request->withHeader('Accept', self::ACCEPT_V2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we'd let external SDK consumers decide which version to use - and we'd decide to only use v2 ourselves. Unless that would cause a lot of extra manual mappings on the SDK end now.
| "MyParcelNL\\Sdk\\": "src/" | ||
| } | ||
| "MyParcelNL\\Sdk\\": "src/", | ||
| "MyParcel\\CoreApi\\Generated\\Shipments\\": "src/Services/CoreApi/Generated/Shipments/lib" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest the prefix here should still be MyParcelNL\Sdk(\CoreApi\Generated\Shipments)?
| "MyParcelNL\\Sdk\\": "src/", | ||
| "MyParcel\\CoreApi\\Generated\\Shipments\\": "src/Services/CoreApi/Generated/Shipments/lib" | ||
| }, | ||
| "exclude-from-classmap": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
double check from my end: this doesn't hinder external use of the generated code?
| command: [ 'composer', 'test:coverage' ] | ||
|
|
||
| generate-api: | ||
| image: openapitools/openapi-generator-cli:latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest switching to an older version for PHP 7.4 compatibility
See https://github.com/OpenAPITools/openapi-generator/pull/17826/changes
Version 7.12.0 should work (instead of latest)
| # Maps inline schema names to shorter, cleaner class names | ||
|
|
||
| inlineSchemaNameMappings: | ||
| # Capabilities V2 - used in CapabilitiesMapper |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest adding the v2 somewhere in the class name to prevent a possible collision with v1 or potential future v3 names.
Implements a Capabilities API client to enable dynamic discovery of carrier-specific functionality, preparing the SDK architecture for future configuration refactoring.
Implementation
Generated Core API client from the OpenAPI spec
Output in src/Services/CoreApi/Generated/Shipments
Generated via Docker using OpenAPI Generator
Regeneration via composer generate:api (generated code committed)
SDK integration
HttpCapabilitiesClient, CapabilitiesMapper, CapabilitiesClientFactory
CapabilitiesService and CapabilitiesServiceInterface
Models
CapabilitiesRequest (immutable request builder)
CapabilitiesResponse (read-only access)
Coverage
Full request support for Core API v2 fields (sender, options, physicalProperties)
Response mapping for package types, delivery types, and shipment option keys
Enums
Removed manual SDK enums
Uses generated Core API enums (RefTypesCarrierV2, RefTypesDeliveryTypeV2, etc.) for validation and normalization
Tooling and CI
Docker-based OpenAPI generation via existing composer script
Added a GitHub Actions workflow that periodically regenerates the client and opens a PR when changes are detected
Tests
Unit tests
Live smoke tests (skipped without API keys)
Usage Examples:
Recommended: Via Service
Direct Client Access
INT-1054