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

client-cli: set response types to undefined for 204 responses, support 201 and empty objects #958

Merged
merged 3 commits into from
May 7, 2023

Conversation

gunjam
Copy link
Contributor

@gunjam gunjam commented May 6, 2023

Hi,

In #884 I added support for 204 responses with no body to the client plugin, the changes in this PR allow the client-cli to generate undefined types for those responses rather than throw because of a lack of a 200 response.

Example:

{
  "responses": {
    "204": {
      "description": "Delete an task template",
      "content": {
        "*/*": {
          "schema": {
            "type": "object"
          }
        }
      }
    }
  }
}

Gives:

interface Tasks {
  deleteTaskTemplate(req: DeleteTaskTemplateRequest): Promise<undefined>;
}

I've also allowed 201 responses and skip attempting to add interface properties if there's no schema defined for the respone (again to prevent client-cli throwing in this case). This is due to having a bunch of Java/Spring APIs in my organisation that return 201 respones with empty object bodies, example openapi schema I'm dealing with:

{
  "201": {
    "description": "Agent created successfully",
    "headers": {
      "Location": {
        "description": "Location URL of the newly created agent.",
        "style": "simple"
      }
    },
    "content": {
      "application/json": {}
    }
  }
}

Gives:

interface CreateAgentResponse {
}

@gunjam
Copy link
Contributor Author

gunjam commented May 6, 2023

Forgot to add tests!

I tried adding a 204 endpoint to the movies fixture (like I did for the client change), but the openapi JSON that is generated has a 200 response:

{
  "/movies/{id}/{title}": {
    "put": {
      "operationId": "updateMovieTitle",
      "parameters": [
        {
          "schema": {
            "type": "string"
          },
          "in": "path",
          "name": "id",
          "required": true
        },
        {
          "schema": {
            "type": "string"
          },
          "in": "path",
          "name": "title",
          "required": true
        }
      ],
      "responses": {
        "200": {
          "description": "Default Response"
        }
      }
    }
  }
}

The plugin has the following code, what am I missing? 🤔

app.put('/movies/:id/:title', {
  schema: {
    operationId: 'updateMovieTitle',
    params: {
      type: 'object',
      properties: {
        id: { type: 'string' },
        title: { type: 'string' }
      },
      required: ['id', 'title']
    },
    responses: {
      204: {
        description: 'Successuly updated title'
      }
    }
  }
}, async (request, reply) => {
  await app.platformatic.entities.movie.save({
    fields: ['id', 'title'],
    input: {
      id: request.params.id,
      title: request.params.title
    }
  })
  reply.status(204)
})

@gunjam
Copy link
Contributor Author

gunjam commented May 6, 2023

I found the answer 😄

gunjam added 2 commits May 7, 2023 12:36
Signed-off-by: Niall Molloy <niall.molloy@engineering.digital.dwp.gov.uk>
Signed-off-by: Niall Molloy <niall.molloy@engineering.digital.dwp.gov.uk>
Signed-off-by: Niall Molloy <niall.molloy@engineering.digital.dwp.gov.uk>
@gunjam
Copy link
Contributor Author

gunjam commented May 7, 2023

Dunno if I'm getting carried away but I decided to instead refactor in an attempt to support all 200 level response codes, including handling sceanrios where an API may have multiple types of success response.

In the case of multiple responses, I've appended the status to the interface name to avoid clashes.

As an example, an API I work with would produce the following types:

interface GetTaskResponseOK {...}
interface CreateTaskResponseCreated {...}
interface ClaimNextTaskResponseOK {...}

interface Tasks {
  getTask(req: GetTaskRequest): Promise<GetTaskResponseOK>; // 200
  createTask(req: CreateTaskRequest): Promise<CreateTaskResponseCreated>; // 201
  // This API returns 200 or 204 if there are no tasks left to claim
  claimNextTask(req: ClaimNextTaskRequest): Promise<ClaimNextTaskResponseOK | undefined>;  
}

Please let me know what you think.

Btw thanks for taking the time to review my changes over the last few weeks, I appreciate you're busy building platformatic and the features I need to integrate in my own organisation may not align with your own goals.
✨ ❤️ 🙏

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina
Copy link
Member

mcollina commented May 7, 2023

Good job! We are pretty busy with the conference season and all the features for the various launches.

Supporting folks adopting Platformatic is why we are here after all! Happy to have a private chat on how we can support you best.

@mcollina mcollina merged commit 8faf801 into platformatic:main May 7, 2023
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants