[vtadmin-web] Add simple /schema view with table definition#7615
[vtadmin-web] Add simple /schema view with table definition#7615rohit-nayak-ps merged 5 commits intovitessio:masterfrom
Conversation
There was a problem hiding this comment.
I have been idly wondering if "Table not found" is confusing given that the route is /schema/.... I guess "schema" is a fairly overloaded term in general. 🤔 Definitely open to suggestions.
There was a problem hiding this comment.
The wording I went with for FindSchema was "no schema found with table %s"
doeg
changed the title
Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
doeg
force-pushed
the
sarabee-vtadmin-web-schema
branch
from
a199423 to
5aeb9ac
Compare
ajm188
left a comment
ajm188
left a comment
There was a problem hiding this comment.
lg2m! a couple non-blocking questions
| return json as HttpResponse; | ||
| // Throw "not ok" responses so that react-query correctly interprets them as errors. | ||
| // See https://react-query.tanstack.com/guides/query-functions#handling-and-throwing-errors | ||
| if (!json.ok) throw new HttpResponseNotOkError(endpoint, json); |
There was a problem hiding this comment.
"NotOkError" seems redundant to me, how about either throw HttpResponseNotOk or throw HttpResponseError ? (also if this is "just a react/ts convention" please let me know!)
There was a problem hiding this comment.
(also i know this is a type from previous PRs ....... which i probably approved without comment so .........)
There was a problem hiding this comment.
I agree it's redundant but I do find it descriptive. This error is specifically when response.ok (in the JSON response envelope) is false.
There are other kinds of "HTTP response errors", like MalformedHttpResponseError which occurs when the response envelope is an unexpected shape. (In all cases, the Error suffix is there since it extends the base Error class.)
I added a commit with bit of commentary on the above. If you still feel like HttpResponseNotOkError is redundant, I can change it.
There was a problem hiding this comment.
Cool, this makes sense to me!
There was a problem hiding this comment.
The wording I went with for FindSchema was "no schema found with table %s"
| <td>{row.keyspace}</td> | ||
| <td>{row.tableDefinition?.name}</td> | ||
| <td> | ||
| <Link to={`/schema/${row.cluster?.id}/${row.keyspace}/${row.tableDefinition?.name}`}> |
There was a problem hiding this comment.
Does this generate an invalid link if row.cluster and row.tableDefinition aren't defined?
There was a problem hiding this comment.
Yeahhhh... so this is one of the annoying things about all proto fields being optional, even when they basically never are in practice. Validating all the pieces all the time can be so cumbersome.
In this case though, I was just being lazy. I've updated it so that in the rare instance that we can't generate a link, the table name will be rendered as plain text (assuming it's defined... which, y'know, it almost certainly will be).
Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
|
@ajm188 thanks for the comments. I think I addressed them all. Here's what the placeholder looks like based on your suggestion:
|
Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
doeg
force-pushed
the
sarabee-vtadmin-web-schema
branch
from
a56b854 to
80f6d29
Compare
ajm188
left a comment
ajm188
left a comment
There was a problem hiding this comment.
Sorry I was slow on the reply. This looks good to me!
| return json as HttpResponse; | ||
| // Throw "not ok" responses so that react-query correctly interprets them as errors. | ||
| // See https://react-query.tanstack.com/guides/query-functions#handling-and-throwing-errors | ||
| if (!json.ok) throw new HttpResponseNotOkError(endpoint, json); |
There was a problem hiding this comment.
Cool, this makes sense to me!
Signed-off-by: Sara Bee <855595+doeg@users.noreply.github.com>
|
Just had to push a sneaky little CSS change for really wide table definitions. 😊 (This will change when we add vindex + column information anyway.) |
4 participants
Signed-off-by: Sara Bee 855595+doeg@users.noreply.github.com
Description
Pretty straightforward! Only instruments the loading + error handling behaviour for now.
Eventually, this page will show things like:
I'll add syntax highlighting later on. I'm not super happy with any of the options + want to be thoughtful about it.
OK
404
Error
Related Issue(s)
Checklist
Deployment Notes
N/A
Impacted Areas in Vitess
Components that this PR will affect: