-
Notifications
You must be signed in to change notification settings - Fork 3k
[CORE] Specification for an HTTP REST catalog #3561
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
Changes from all commits
eafed3d
fb81cfb
dda40a3
28a4f9f
495c4c5
08cc6a9
eebb775
484a573
12a61be
dea7a62
54263ae
3a1bb09
4aecdda
42834b3
fd1591c
a4e2e59
20aa1c1
46f8c1a
cee5dc0
58f94e3
3fa7bcd
28d44a2
8044e56
460a57b
1af20b3
56fff46
b2c10c9
651c419
f3e65df
12622c7
5e342fd
4fe74d2
e3cc497
2b396b7
03a3708
6ef7944
80501ff
06d1de6
bf42455
a71b6c9
7f0e848
fd77e47
2e87e72
e11605e
e95b5ab
efc6acf
1f35862
33129b8
540af23
73cfb0d
db52c0a
1171e08
716c421
85ed09f
5768a72
d698f44
c7df485
87a96ce
a0084ec
7f36e31
03db9b7
e67f9a8
a15ffba
7a1dd9a
5292a1d
c83a78f
a6bf1ea
55db1ea
19ed45b
9771cd9
9bbea49
9c388ab
291fc3f
ea301eb
17ed2df
9d1b3de
0d998e5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,121 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one | ||
| * or more contributor license agreements. See the NOTICE file | ||
| * distributed with this work for additional information | ||
| * regarding copyright ownership. The ASF licenses this file | ||
| * to you under the Apache License, Version 2.0 (the | ||
| * "License"); you may not use this file except in compliance | ||
| * with the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, | ||
| * software distributed under the License is distributed on an | ||
| * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY | ||
| * KIND, either express or implied. See the License for the | ||
| * specific language governing permissions and limitations | ||
| * under the License. | ||
| */ | ||
|
|
||
| package org.apache.iceberg.rest.http; | ||
|
|
||
| import com.fasterxml.jackson.annotation.JsonCreator; | ||
| import com.fasterxml.jackson.annotation.JsonInclude; | ||
| import com.fasterxml.jackson.annotation.JsonProperty; | ||
|
|
||
| /** | ||
| * | ||
| * NOTE: This document isn't intended as part of the Spec, and was | ||
| * intended for showing something to somebody. As there is on-going | ||
| * discssion, I'm leaving it, but then I will likely remove it for now. | ||
| * | ||
| * All responses for the first version of the Iceberg REST API will be JSON objects, a top-level envelope object with two fields, | ||
| * `error` and `data`, which are themselves JSON objects. | ||
| * | ||
| * data: represents the JSON encoded response for a successful path or for a valid / expected failure. JSON representation of various response types | ||
| * error: a standardized obect on error, containing the error code, message, type, and additional metadata (an optional JSON object of metadata) as defined below. | ||
| * | ||
| * All responses for the REST catalog should be wrapped this way, vs using primitives. For example, for a ListTableResponse, listing tables under a namespace "accounting", | ||
| * we'd get a JSON object back like the following: | ||
| * (the field `error`, having a value of null, has not been serialized). | ||
| * | ||
| * { "data": { "identifiers": [ "accounting.tax", "accounting.currency_conversions"] } } | ||
| * | ||
| * NOTE: The direction now is to head to just using HTTP response codes and | ||
| * not internal codes, as we have `Type`. If anybody feels strongly about | ||
| * using internal codes, please bring that up. | ||
| * | ||
| * If the namesapce `accounting` didn't request, the response from that call would have a body like the following, | ||
| * where the `code` 40401 is a two-part identifier: | ||
| * - HTTP response code: 404 | ||
| * - Two digit internal application defined error code for further detail: 01 - Namespace not found. | ||
| * | ||
| * { "data": {}, "error": { "message": "Failed to list tables. The Namespace 'accounting' does not exist", "type": "NamespaceNotFoundException", "code": 40401 } | ||
| * | ||
| * We could also embed the HTTP response code plainly by itself, without internally documented codes, as a separate field. I have found having a documented list of internal | ||
| * error codes to be very helpful previously, but am open to discussion on this. | ||
| */ | ||
| @JsonInclude(JsonInclude.Include.NON_NULL) | ||
| public class IcebergHttpResponse<T> { | ||
|
|
||
| private final T data; | ||
| private final Error error; | ||
|
|
||
| @JsonCreator | ||
| public IcebergHttpResponse( | ||
| @JsonProperty("data") T data, | ||
| @JsonProperty("error") Error error) { | ||
| this.data = data; | ||
| this.error = error; | ||
| } | ||
|
|
||
| public Error error() { | ||
| return error; | ||
| } | ||
|
|
||
| public T data() { | ||
| return data; | ||
| } | ||
|
|
||
| /** | ||
| * An error object embedded in every HTTP response. | ||
| * | ||
| * On error, this contains: | ||
| * - message: A short, human-readable description of the error. | ||
| * - type: Type of exception - more specifically a class name, e.g. NamespaceNotFoundException) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this have to be part of the spec? In other words are specific exception class names going to be part of the spec in addition and/or in parallel to "internal" error codes?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No this isn't part of the spec at all. I uploaded this here to share it with somebody I was speaking to, but this file is not part of the spec whatsoever. Sorry about the confusion! |
||
| * - code: An (optional) application specific error code, to distinguish between causes | ||
| * of the same HTTP response code (eg possibly different types of Unauthorized exceptions). | ||
| * | ||
| * #################### Optional fields to consider ###################################### | ||
| * - status: HTTP response code (optional). | ||
| * - traceId: Unique specific identifier for this error and request, for monitoring purposes. | ||
| * Presumably this would be an OpenTracing Span (optional). | ||
| * Will almost certainly add tracing headers as an optional follow-up. | ||
| * - metadata: Further map of optional metadata (such as further directions to users etc) (optional - unsure?). | ||
| * ####################################################################################### | ||
| * | ||
| * Example: | ||
| * "error": { | ||
| * "message": "Authorization denied: Missing Bearer header", | ||
| * "type": "OAuthException", | ||
| * "code": 40102 | ||
| * } | ||
| */ | ||
| public static class Error { | ||
|
|
||
| private final String message; | ||
| private final String type; | ||
| private final int code; | ||
|
|
||
| @JsonCreator | ||
| public Error( | ||
| @JsonProperty("message") String message, | ||
| @JsonProperty("type") String type, | ||
| @JsonProperty("code") int code) { | ||
| this.message = message; | ||
| this.type = type; | ||
| this.code = code; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,3 +24,5 @@ gradle/* | |
| package-list | ||
| sitemap.xml | ||
| derby.log | ||
| rest_docs/* | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it be clearer if the HTTP error code were separate from the internal error code? This would allow free form internal code without having to be confined to 100 values under an HTTP code prefix, and at the same time maintain reference to the more generic HTTP set of codes. It would also make it easier to extract HTTP error codes for processing.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So since we have
type, which can be used to enumerate all of the possible sub fields, we've moved in the direction of just using the 3-digit HTTP response code in the body.This file isn't part of the spec at all, as mentioned below, so I forgot to update it. I will remove it eventually but leave it so we can discuss.
Do you have input on using a dedicated int
code(perhaps not constrained to HTTP code space as you mentioned) instead of just using the standardized value intype(which is a string)?If we used
code, the lookup table in my experience brings you totype. But I have previously used the way you described, with numeric error codes that we define a lookup table for that are independent. Do you have feedback on moving to justtype?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO,
typeis preferable to numericcode, but it might be harder to define strictly and there's risk thattypenames deviate from actual exception class names. Plus fixing class names in the API spec might affect non-java implementations in a negative way and complicate java code refactoring too.In Nessie, as an example, we have an enum of non-numeric error codes. The API returns enum names, but they can be mapped to java exceptions upon parsing. Python client has its own mapping too. cf. ErrorCode.java
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Then we'll stick with
typefor now and use the http response codes forcode.For the moment, there's no hard requirement that they map to exception names. And this class isn't actually part of the spec, it was more to show an idea to somebody.
We don't have to use class names / exception names. For now, let's just say that they are a set of well-defined names (at least for any values that we might want the client to react to).
Though implementations could in theory use a different set of names and they might have benefit for logging purposes / monitoring purposes, but the client wouldn't be able react to any custom names (because it wouldn't know about them and how to react to them). We don't ideally want to restrict the spec so much that people can't make it suit their needs (for example, if having a richer set of
typewould help them with observability in the future).We can cover what those values are called in a separate discussion later on (this is far from the only discussion / reviewable that will be had about this so I think it's safe to punt it down the road, with that knowledge in mind that that's the general goal for now).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excluding this class from the spec and using only OpenAPI yaml SGTM 👍
Re:
typevalues, I personally tend to think that if the field itself is part of the spec, the spec would have to define how to interpret it. I am not sure keeping it "opaque" is going to be that much useful from the spec's perspective.Is this spec intended to define just a common part of the protocol between a Catalog impl. on the Iceberg side and the HTTP server that offers the REST API? Are the client and server parts supposed to be interoperable or are they assumed to work knowing the exact nature and version of the other side?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Since we're using code to just be the HTTP status response code, I would say that
typeshould be a well defined set we can look up.We might also choose to allow people to use others that will get ignored by the client but are logged or something.
But for now I would say let's plan for it to be a well defined set of values, but not necessarily attempt to enumerate all of them just yet since we don't have about half of the basic Catalog AP in here yet. It's currently mostly just around namespaces and some of the table related ones will be handled next.
Sound good?