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

daemon: the output format for api #376

Open
FeelyChau opened this issue Jul 27, 2020 · 20 comments
Open

daemon: the output format for api #376

FeelyChau opened this issue Jul 27, 2020 · 20 comments

Comments

@FeelyChau
Copy link
Collaborator

FeelyChau commented Jul 27, 2020

Current output data format:

ctx.body = {
  ...data,
  status: false,
};

There are some problems:

  1. The data field is destructed, so the output data is dispersed, which makes the client a little cumbersome to handle.
  2. Error message is missing. If we got any error, the error message filed name depends on the specific api. Maybe we should offer a generic error field.

What I prefer:
Output data format:

{
  "data": {},
  "message": "success",
  "code": 200
}
field name type description
data any the data returned
message string the message
code number error code (http code?)

data struct is defined by specific apis.

@rickycao-qy
Copy link
Collaborator

do we need to return code inside response body? Cuz we can directly set HTTP status code

@FeelyChau
Copy link
Collaborator Author

FeelyChau commented Jul 27, 2020

We could define some codes to represent some special error types.
For example:
-100: pipeline not exists.
-101: pipeline is not initialized.

@rickycao-qy

@WenheLI
Copy link
Collaborator

WenheLI commented Jul 27, 2020

@rickycao-qy - I think we can also use some custom status code to indicate errors in our plugins.
I.E.

1 -> Data Collect Error
2 -> Data Process Error
...

@yorkie
Copy link
Member

yorkie commented Jul 27, 2020

Why not using HTTP Message and StatusCode instead? HTTP provides the extensible way for code and messages, see https://tools.ietf.org/html/rfc2616#section-6.1.1.

@FeelyChau
Copy link
Collaborator Author

Why not using HTTP Message and StatusCode instead? HTTP provides the extensible way for code and messages, see https://tools.ietf.org/html/rfc2616#section-6.1.1.

Depend on how we define the message. If we defined the error code and message is application-level, put them into the header is reasonable, or they work on business-level, then body is better.
And, the code in the body is more flexible to represent custom-status.

@yorkie
Copy link
Member

yorkie commented Jul 27, 2020

HTTP is the application-level protocol, and the current definition looks a little bit duplicated.

@FeelyChau
Copy link
Collaborator Author

Yea, the message and code we needed looks more like a business information, the error expressed in http code is a little abstract to a business api.

@yorkie
Copy link
Member

yorkie commented Jul 27, 2020

Shall we have a design in a more specific problem, just like we can list the request/response schema of all renew APIs, therefore some potential standard formats will be vivid?

@yorkie
Copy link
Member

yorkie commented Jul 27, 2020

Another specific TODO before proposing the design is to list all errors and corresponding codes/message pairs just like what you were doing at #376 (comment).

@FeelyChau
Copy link
Collaborator Author

FeelyChau commented Jul 27, 2020

We can list some errors, and confirm how they can be defined or if they should be defined.
I think no need to list all of them, it's enough to enumerate some typical errors.
I'll collect the errors and put them here.

@yorkie
Copy link
Member

yorkie commented Jul 27, 2020

We can list some errors, and confirm how they can be defined or if they should be defined.
I think no need to list all of them, it's enough to enumerate some typical errors.
I'll collect the errors and put them here.

That's it, looking forward to that!

@FeelyChau
Copy link
Collaborator Author

We can list some errors, and confirm how they can be defined or if they should be defined.
I think no need to list all of them, it's enough to enumerate some typical errors.
I'll collect the errors and put them here.

That's it, looking forward to that!

Actually, the HTTP MESSAGE is put in the response body too, it's the same for api users. If we can confirm the HTTP CODE is applicative, the output could be like this:

{
  "message": "something error",
  "error": {
    "type": "resource",
    "code": "pipeline_not_exists"
  }
}

with HTTP CODE 400.

@yorkie
Copy link
Member

yorkie commented Jul 28, 2020

What does the code and type mean?

@FeelyChau
Copy link
Collaborator Author

What does the code and type mean?

Just more detail for the error, it can be any field needed.

This was referenced Jul 28, 2020
@FeelyChau
Copy link
Collaborator Author

FeelyChau commented Jul 30, 2020

I think we need versioning for the daemon api.
There are some cases for versioning in RESTful api.
If we include the version in the URI, it will look like: http://pipcook.com/api/v1/job .
I will implement in #380 if there is no problem.

@yorkie
Copy link
Member

yorkie commented Jul 30, 2020

I don't think the API versioning should be included at Pipcook, it depends on users IMO.

@FeelyChau
Copy link
Collaborator Author

Without versioning, once the apis used by any user, the api will be frozen.
Unless we want to bind the server and client application, but I think it's unreasonable.

@yorkie
Copy link
Member

yorkie commented Jul 30, 2020

We are not to bind the service and client, the developer is able to deploy what they prefer to choose, and using the corresponding SDK. And maintaining multiple versions in single branch is hard, too.

@Txiaozhe
Copy link
Collaborator

May be define some error code for product is better, such as
'0' means 'success',
'1' means some params are error
....
Product error codes should be distinguished from HTTP error codes:)

@FeelyChau
Copy link
Collaborator Author

FeelyChau commented Jul 31, 2020

@Txiaozhe The custom-code will be set to the http message, if need. Refer to Github API

@yorkie yorkie changed the title Daemon: the output format for api daemon: the output format for api Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants