Redundant "data" field in API spec (collections.yaml)

[stargazer33]

Version:
latest, see https://listmonk.app/docs/swagger/collections.yaml

After fixing #1317 still there are problems in the *.yml file.

Now the data field added to JSON response (everywhere?). This is great.
What is not great - the data field still exists in some response objects/structures.
It looks like the response should contain the data two times, which is not the case!

I know about these places:

These two places: it is definetly a bug:
Line 3106 (in collections.yaml):

    Campaign:
      type: object
      properties:
        data:
          type: object
          properties:

Line 3168:

    NewCampaign:
      type: object
      properties:
        data:

These two places: I'm not sure, please compare with real JSON response/test:
Line 2751:

    Settings:
      type: object
      properties:
        data:

    Bounce:
      type: object
      properties:
        data:

Some additional info
Having collections.yaml is a great help, thanks to the authors!
I'm using it to generate client API in Java (need this API to talk to Listmonk from my program. I'm using/creating subscribers, campaigns and lists, nothing else)

It seems, this API spec -I mean the *.yml file- is not used in development of Listmonk server/client code (otherwise these bugs would be fixed very quickly).
I would suggest to setup some (half)automated check to compare the API described in *.yml with real JSONs used on server/client side.
Without such check - it is a never ending story, there always be bugs

There are tools for this, look here: https://openapi.tools/#description-validators
One of these tools, vacuum written in Go by the way :-)

[knadh]

@stargazer33 since you're able to spot these issues and you're familiar with the spec, would greatly appreciate it if you could send a PR with the fixes. Will save compared to time passing these on to @rohansh-tty who is the original contributor of the collection. Thanks!

[stargazer33]

@stargazer33 since you're able to spot these issues ...

@knadh - I am focused only on small subset of Subscriber+List+Campaign calls, nothing else.
I see these issues because I am writing code doing these API calls (using generated Java classes) and from time to time I discover these issues.

It should be a task of the original contributor to think how to fix ALL such issues (my suggestion - to setup automation)

[stargazer33]

One more problem:

I want to add list 12 to subscriber 23 (JSON below generated based on the API spec in collections.yml - I am not writing it manually):

PUT /api/subscribers/lists
{
  "ids": 23,
  "action": "add",
  "target_list_ids": 12,
  "status": "unconfirmed"
}

and Listmonk response is:

400 Bad Request /api/subscribers/lists 
{
  "message": "One or more IDs are invalid: code=400, message=Unmarshal type error: expected=[]int, got=number, field=ids, offset=9, internal=json: cannot unmarshal number into Go struct field subQueryReq.ids of type []int"
}

I think, the problem is in collections.yml, line 316 - the "ids" field is NOT an array (please check definition of other fields too!):

  /subscribers/lists:
    put:
      description: handles bulk addition or removal of subscribers
      tags:
        - Subscribers
      requestBody:
        content:
          application/json:
            schema:
              type: object
              properties:
                ids:
                  type: number
                  description: The ids of the subscribers to be modified.

UPDATE: the target_list_ids should be an array too.

And may be some other places in *.yml have this problem too, pls check...

[stargazer33]

And again one more:

The Subscriber.id field is sometimes integer and sometimes number
I have no idea which one is better, just make it consistent, I do not want to convert it on the client side.

Here (and in many other places!) it is an integer: Line 473:

  "/subscribers/{id}/optin":
    post:
      description: sends an optin confirmation e-mail to a subscriber.
      tags:
        - Subscribers
      parameters:
        - in: path
          name: id
          required: true
          description: sends an optin confirmation e-mail to a subscriber
          schema:
            type: integer

Here it is a number: Line 2940:

    Subscriber:
      type: object
      properties:
        id:
          type: number

[stargazer33]

@knadh @rohansh-tty
I'm testing this rohansh-tty@3593fc0

Can not say about functionality... yet...
What I see - this is a HUGE change!
Well, one day you have to fix the API - I understand it.

But, for the future - you have to avoid such huge API changes!!!
Because other developers use this API - they write clients based on API spec in *.yml

I wrote two (playground) clients for this API and what I see - this change broke them. Have to rename many methods.
So, for the future - please be more careful with the API, avoid breaking changes if possible

[hkirat]

Hi, I was going through the changes that introduced the API spec.
Out of curiosity, I wanted to understand how the file was generated? Was it auto-generated somehow?

From my (very limited) knowledge in Golang, there's no easy way to auto-generate the spec file without adding documentation through the project (ref https://github.com/swaggo/echo-swagger).

Would it make sense to introduce something like this through the codebase so the file is auto-generated.
There would still be scope for error since the file isn't generated automatically from the code (which from what I can tell is not possible in Golang), but would make it slightly easier to track and make changes vs making them directly in the spec file.

[knadh]

But, for the future - you have to avoid such huge API changes!!!
Because other developers use this API - they write clients based on API spec in *.yml

Hi @stargazer33. Sorry to hear that. From what I've learnt here, the Swagger collection has a lot of issues. Basically, wrong spec/wrong API definitions. So the APIs shouldn't have worked in the first place. They are getting corrected now. How was it working in your setup?

Out of curiosity, I wanted to understand how the file was generated? Was it auto-generated somehow?

From what I know @rohansh-tty has created the collection manually. If there's a straight forward way to generate it automatically from the code itself, that'd be the ideal way forward to keep the API + collection in sync. I have never had time to explore this unfortunately. The tool you've shared looks interesting, although it'll have to be run manually every time there's a change. Will check this out.

[stargazer33]

... there's no easy way to auto-generate the spec file without adding documentation through the project (ref https://github.com/swaggo/echo-swagger).
Would it make sense to introduce something like this through the codebase so the file is auto-generated.

Yes, either this way...

OR: you can go another way: keep the current *.yml and... validate the HTTP+JSON of the actual requests+responses on the server side against the specification in the *.yml file.

Something like this:
https://github.com/getkin/kin-openapi#validating-http-requestsresponses

As far as I understand - these ValidateRequest, ValidateResponse methods - they return errors if something goes wrong.
I can imagine some kind of "validation" mode (a command-line switch?) of Listmonk...
And you should do some test runs and check for validation errors

P.S. I'm not a Go programmer, but somehow I (half)understand the code )))

[stargazer33]

OK, this: rohansh-tty@3593fc0 looks good (much better than before), but there still can be errors - I am testing only a few API calls

[stargazer33]

Ok, the problem in the #1325 happens because the URL in the collections.yml is wrong.

this code works

curl -u "listmonk:password!" "http://localhost:9001/api/tx" -X POST -H 'Content-Type: application/json; charset=utf-8' --data-binary @- << EOF
    {
        "subscriber_email": "test0003@mycooltestdomain.com",
        "template_id": 3
    }
EOF

So it should be /tx not /transactional in the *.yml

[stargazer33]

One more:

The TransactionalMessage, (line 3445) should be fixed, it is missing the content_type, messenger input parameters mentioned in documentation:

    TransactionalMessage:
      type: object
      properties:
        subscriber_email:
          type: string
        subscriber_id:
          type: integer
        template_id:
          type: integer
        from_email:
          type: string
        data:
          type: object
        headers:
          type: array
          items:
            type: object