Skip to content

Fix Swagger-generated operation parameter names for "body" parameters #7446

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

Merged
merged 1 commit into from
Jul 12, 2017
Merged

Fix Swagger-generated operation parameter names for "body" parameters #7446

merged 1 commit into from
Jul 12, 2017

Conversation

careys7
Copy link
Member

@careys7 careys7 commented Nov 16, 2016

These were previously all output as a string $body. This improves Swagger-generated API clients and compatibility with swagger-codegen & janephp/openapi generator.

The previous naming caused the following issues:

  • Variable variable output in janephp/openapi code generation (due to the $ output as string)
  • Incorrect code output (multiple models called Body) in janephp/openapi
  • "Anonymous" class naming in swagger-codegen generated clients (approx 20 models called Body1, Body2, Body3 are output for a Magento Enterprise schema)

The new format is: operationNamePostBody. Operation Id is guaranteed to be unique under the Open API specification. Changing the body parameter name in the schema should not affect clients conforming to the specification.

These were previously all output as a string "$body". This improves Swagger-generated API clients and compatibility with swagger-codegen & janephp/openapi generator.

This caused the following issues:

- Variable variable output in janephp/openapi code generation
- Incorrect code output (multiple models called "Body") in janephp/openapi
- "Anonymous" class naming in swagger-codegen generated clients (approx 20 models called Body1, Body2, Body3 are output for a Magento Enterprise schema)

The new format is: operationNamePostBody. Operation Id is guaranteed to be unique under the Open API specification. Changing the body parameter in the schema should also not affect clients conforming to the specification.
@MomotenkoNatalia MomotenkoNatalia added the Component: Framework/Webapi USE ONLY for FRAMEWORK RELATED BUG! E.g If bug related to Catalog WEB API use just Catalog label Dec 14, 2016
@fooman fooman self-assigned this Jun 30, 2017
@fooman
Copy link
Contributor

fooman commented Jun 30, 2017

hey @careys7 you still interested in getting this merged?

My main question would be how likely are we going to break existing clients, ie if someone had used Body3?

What are the best steps to reproduce the difference this PR makes?

@careys7
Copy link
Member Author

careys7 commented Jul 2, 2017

hey @fooman - yes definitely.

The best steps to reproduce are to compare the diff of the two schemas, or run code generation using swagger-codegen / janephp openapi against them. swagger-codegen is probably a good start as it supports multiple languages which makes it a more likely choice for third parties integrating with Magento.

Keep in mind that the search criteria implementation will not work (nor is it ever expected to in its current form) in clients generated using either of them (see #7511). This PR improves the model classes which makes up the bulk of the generation needed for a lot of API projects. The API resources themselves need processing after generation to add proper support for search criteria.

Existing clients will notice that all of the "Body" classes will be renamed if they re-generate their model from schema, but will not notice any functional changes otherwise (in terms of their client still being operable with Magento).

See below for some examples of the changes in naming:

Before (Random Sample)

> cd /path/to/Model && ls | grep Body

Body
Body1
Body10
Body116
Body12
Body120

After (Random Sample)

> cd /path/to/Model && ls | grep Body

QuoteGuestCartItemRepositoryV1SavePostBody
QuoteGuestCartManagementV1AssignCustomerPutBody
QuoteGuestShipmentEstimationV1EstimateByExtendedAddressPostBody
QuotePaymentMethodManagementV1SetPutBody
QuoteShipmentEstimationV1EstimateByExtendedAddressPostBody
RmaCommentManagementV1AddCommentPostBody
RmaTrackManagementV1AddTrackPostBody
SalesCreditmemoCommentRepositoryV1SavePostBody

The generated classes can also look a lot clearer (the below example is a post-processed version from janephp).

<?php

declare(strict_types=1);

namespace Magento2\Magento2Api\Model;

use JMS\Serializer\Annotation\Type;

class SalesShipOrderV1ExecutePostBody
{
    /**
     * @var bool
     * @Type("boolean")
     */
    protected $appendComment;
    /**
     * @var SalesDataShipmentCreationArgumentsInterface
     * @Type("Magento2\Magento2Api\Model\SalesDataShipmentCreationArgumentsInterface")
     */
    protected $arguments;
    /**
     * @var SalesDataShipmentCommentCreationInterface
     * @Type("Magento2\Magento2Api\Model\SalesDataShipmentCommentCreationInterface")
     */
    protected $comment;
    /**
     * @var SalesDataShipmentItemCreationInterface[]
     * @Type("array<Magento2\Magento2Api\Model\SalesDataShipmentItemCreationInterface>")
     */
    protected $items;
    /**
     * @var bool
     * @Type("boolean")
     */
    protected $notify;
    /**
     * @var SalesDataShipmentPackageCreationInterface[]
     * @Type("array<Magento2\Magento2Api\Model\SalesDataShipmentPackageCreationInterface>")
     */
    protected $packages;

// etc

As well as usages in calling code:

Before

$postBody = new Body123();
$postBody->setNotify(true);
$postBody->setTracks($tracks);
$postBody->setPackages($packages);
// etc

After

$postBody = new SalesShipOrderV1ExecutePostBody();
$postBody->setNotify(true);
$postBody->setTracks($tracks);
$postBody->setPackages($packages);
// etc

Existing projects that make a Magento 2 Swagger-generated API publicly available as part of their source may choose to make use of sub-classing to maintain backwards compatibility with implementations that use their package, for example:

<?php
class Body123 extends SalesShipOrderV1ExecutePostBody {}

This could otherwise be considered a breaking change for those libraries when their release contains a newly generated Magento API client.

@fooman fooman added this to the July 2017 milestone Jul 6, 2017
@fooman fooman added the develop label Jul 6, 2017
@magento-team magento-team added Progress: accept develop Component: Framework/Webapi USE ONLY for FRAMEWORK RELATED BUG! E.g If bug related to Catalog WEB API use just Catalog labels Jul 11, 2017
@magento-team magento-team merged commit 00bcf17 into magento:develop Jul 12, 2017
magento-team pushed a commit that referenced this pull request Jul 12, 2017
magento-team pushed a commit that referenced this pull request Jul 12, 2017
@careys7
Copy link
Member Author

careys7 commented Aug 11, 2017

@magento-team is there a target release for this PR?

@fooman
Copy link
Contributor

fooman commented Aug 11, 2017

@careys7 2.2 - if you want it in the 2.1 branch you would need to open a separate PR

@careys7
Copy link
Member Author

careys7 commented Aug 11, 2017

@fooman
Copy link
Contributor

fooman commented Aug 11, 2017

Sorry indeed I would have expected it to be in there as well. Let me inquire and get back to you.

@careys7
Copy link
Member Author

careys7 commented Aug 15, 2017

@fooman any news?

@fooman
Copy link
Contributor

fooman commented Aug 15, 2017

@careys7 so far got two different answers - still checking

@fooman
Copy link
Contributor

fooman commented Aug 16, 2017

@careys7 apologies looks like this is not going to make 2.2.0, I'll get back to you when I know more about 2.2.1

@careys7
Copy link
Member Author

careys7 commented Aug 18, 2017

ok thanks @fooman

okorshenko pushed a commit that referenced this pull request Nov 10, 2017
magento-team pushed a commit that referenced this pull request Nov 20, 2017
@keharper
Copy link
Contributor

keharper commented Dec 7, 2017

Apparently, the Magento Swagger UI has a fixed width, and it doesn't allow you to scroll horizontally. Replacing "body" with <operationName>PostBody can cause the text area on the right side that shows the payload structure to be inaccessible.

2 2 2-swagger-display

Any idea how this can be fixed?

@careys7
Copy link
Member Author

careys7 commented Dec 8, 2017

Version 3 seems to handle this a lot better

Edit: have opened issue here for feedback: #12595

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Framework/Webapi USE ONLY for FRAMEWORK RELATED BUG! E.g If bug related to Catalog WEB API use just Catalog Progress: accept
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants