feat: Add json schema support #231
Conversation
frontend/src/state/backendApi.ts
Outdated
| @@ -741,7 +741,12 @@ const apiStore = { | |||
| const rq = cachedApiRequest(`./api/schemas/subjects/${subjectName}/versions/${version}`, force) as Promise<SchemaDetailsResponse>; | |||
|
|
|||
| return rq | |||
| .then(({ schemaDetails }) => (this.schemaDetails = schemaDetails)) | |||
| .then(({ schemaDetails }) => { | |||
| if ( schemaDetails && typeof schemaDetails.schema === "string" && schemaDetails.type !== "PROTOBUF") { | |||
There was a problem hiding this comment.
I think the typeof check is not really needed as we rely on the correctness of our typescript types, but @rikimaru0345 might chime in here as he's mostly responsible for the frontend code.
There was a problem hiding this comment.
@weeco The main reason I did this was that even for Avro schemas it was not showing the detail in the schema details page, and the issue was because the schema object is sent as string from the backend, adding this validation/conversion solved the issue and showed the schema correctly,
There was a problem hiding this comment.
no, the typeof check is fine.
yes, it is assumed that the backend currently sends schema as a string and not anything else, but defensive programming is always a good idea.
|
@weeco Thanks for all the comments, Yes I tested it against the three types of schemas (JSON, PROTOBUF, and AVRO) |
|
Code looks good to me. |
|
@rikimaru0345 yes, it should show the fields for both Avro and Json schemas |
| if d.SchemaService != nil && len(payload) > 5 && payload[0] == byte(0) { | ||
| schemaID := binary.BigEndian.Uint32(payload[1:5]) | ||
| trimmed := bytes.TrimLeft(payload[5:], " \t\r\n") | ||
| startsWithJSON := trimmed[0] == '[' || trimmed[0] == '{' | ||
| if startsWithJSON { |
There was a problem hiding this comment.
@weeco from my comment in the PR thread: see here
|
@weeco I've also looked through the backend changes and the only thing I noticed is this: https://github.com/cloudhut/kowl/pull/231/files?diff=split&w=1#r671009018 Do you think that is "safe" enough for the detection? I mean, I'd even go as far as saying that - compared to our other "auto detect payload type" checks - this is one of the more robust checks we have in there 😂. So I doubt there's much (anything really) that can be improved there. In any case, the other backend changes seem fine to me as well (though I didn't check them as deeply as the frontend). From my side this PR looks perfectly fine! 👍 |
No description provided.