Skip to content
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

Service subscription API suggestions #3750

Closed
wants to merge 29 commits into from

Conversation

mayorova
Copy link
Contributor

@mayorova mayorova commented Apr 9, 2024

UPDATE [2024-04-16]:

After speaking with @kevprice83 and @3scale/product we thought that it would be better to deprecate the old enpoints and add new ones to replace them, with the improved representation, to avoid any breaking changes for the customers. So this PR has been changed to adapt to it. It was also rebased on top of master to better understand what would be the change compared to the current behavior.


This PR addresses some of the issues that I mentioned in the comments to #3672

JSON (see #3672 (comment)) and XML (see #3672 (comment)) representation

I see @nidhi-soni1104 that you added more properties in d458f31, but this is not actually the full fix:

  • indeed it added extra fields in the show, but still the representation was not the same as for index, compare:
    show:
{
    "service_contract": {
        "id": 4,
        "plan_id": 96,
        "user_account_id": 3,
        "created_at": "2023-04-23T19:27:02Z",
        "updated_at": "2024-04-09T08:50:40Z",
        "state": "live",
        "paid_until": "2024-04-30T23:59:59Z",
        "trial_period_expires_at": "2023-04-23T19:27:02Z",
        "setup_fee": "0.0",
        "type": "ServiceContract",
        "variable_cost_paid_until": "2024-04-09T11:45:35+02:00",
        "tenant_id": 2,
        "service_id": 2
    }
}

index:

{
    "service_contracts": [
        {
            "service_contract": {
                "id": 4,
                "plan_id": 96,
                "user_account_id": 3,
                "user_key": null,
                "provider_public_key": null,
                "created_at": "2023-04-23T19:27:02Z",
                "updated_at": "2024-04-09T08:50:40Z",
                "state": "live",
                "description": null,
                "paid_until": "2024-04-30T23:59:59Z",
                "application_id": null,
                "name": null,
                "trial_period_expires_at": "2023-04-23T19:27:02Z",
                "setup_fee": "0.0",
                "redirect_url": null,
                "variable_cost_paid_until": "2024-04-09T11:46:30+02:00",
                "extra_fields": null,
                "tenant_id": 2,
                "create_origin": null,
                "first_traffic_at": null,
                "first_daily_traffic_at": null,
                "service_id": 2,
                "accepted_at": null
            }
...

There are a bunch of "null" values in index that are not present in show, and the responses must be the same.

Also, many of the properties that appear in the response (specifically the index) are not relevant at all for service subscriptions (description, name, extra fields, first_traffic_* and others), so they had to be removed.

Also, the weird <to_xml/> in the XML representation were not addressed. And actually, I noticed that in general the XML representation is not right, and it doesn't follow the format we are using for other objects.

Compare this:

<?xml version="1.0" encoding="UTF-8"?>
<service-contract>
    <id>4</id>
    <to_xml/>
    <plan-id>96</plan-id>
    <to_xml/>
    <user-account-id>3</user-account-id>
    <to_xml/>
    <user-key nil="true"/>
    <provider-public-key nil="true"/>
    <created-at type="dateTime">2023-04-23T19:27:02Z</created-at>
    <updated-at type="dateTime">2024-04-09T08:50:40Z</updated-at>
    <state>live</state>
    <description nil="true"/>
    <paid-until type="dateTime">2024-04-30T23:59:59Z</paid-until>
    <application-id nil="true"/>

to this (current representation of an application):

<?xml version="1.0" encoding="UTF-8"?>
<application>
    <id>380</id>
    <created_at>2023-06-28T10:58:10Z</created_at>
    <updated_at>2023-07-03T13:22:37Z</updated_at>
    <state>live</state>
    <user_account_id>3</user_account_id>
    <first_traffic_at/>
    <first_daily_traffic_at/>
    <service_id>2</service_id>

Note the underscores vs dashes in the property names, attributes in some of the elements (e.g. type="dateTime".

The bad news is that we were already using this wrong representation in the previously existing index endpoint of the API, so this would be a breaking change. On the other hand, I think the sooner we fix it the better - using inconsistent XML format for a single endpoint is pretty bad, IMO, I'd consider it a bug.

Errors inconsistencies, see #3672 (comment)

I checked what we do for the applications endpoint, and the errors are represented as:

  • incorrect plan for apps:

    <?xml version="1.0" encoding="UTF-8"?>
      <errors>
        <error>Plan not allowed in this context</error>
      </errors>
    
    {
      "errors": {
          "plan": [
              "not allowed in this context"
          ]
      }
    }
    
  • update user key for apps:

    <?xml version="1.0" encoding="UTF-8"?>
      <errors>
        <error>User key has already been taken</error>
      </errors>
    
    {
      "errors": {
          "user_key": [
              "has already been taken"
          ]
      }
    }
    

This is how the errors generated at model validation look like, so I moved the "same service" validation from the controller to the model. In fact, I think that lacking this validation on the model was a bug, i.e. without the controller validation it was possible to change the subscription to a plan on a different service, which is just wrong (this is not an expected behavior, because, for example, the service_id never gets updated on a service contract, and the UI also doesn't support this).

Change "Update" endpoint to "Change Plan"

I compared how we change plan for accounts and applications, and it's not done via the update endpoint, but rather there is a custom change_plan endpoint, so I changed this for Service Subscriptions as well, for consistency.

I also updated the API Docs and grouped all the Service Subscription-related endpoints under the "Service Subscriptions" tag, as it looked weird under accounts. Now it's better organized.
Screenshot from 2024-04-09 11-33-49

@mayorova mayorova changed the base branch from THREESCALE-2689-Service-Subscription-API to master April 16, 2024 14:35
@mayorova
Copy link
Contributor Author

Superseded by #3759

@mayorova mayorova closed this Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants