[CORE] Add SerDe tests for CreateTableRequest#5052
Conversation
| private UnboundSortOrder order; | ||
| private Map<String, String> properties; | ||
| private Boolean stageCreate; | ||
| private Boolean stageCreate = false; |
There was a problem hiding this comment.
I defaulted this to false so that users didn't need to explicitly set it if writing JSON. Because it's a boxed Boolean, if it's not present in the JSON, it gets set as null.
I did this because CreateTableRequest does not have stageCreate in its list of required fields in the OpenAPI spec.
As there is an assertion that stageCreate is not null in the validate method of this class, I would also be comfortable with requiring that stageCreate be set in the OpenAPI spec, but I prefer this as it matches the builders behavior as well (if .stageCreate() isn't called on the builder, false is used).
| @Test | ||
| // Test cases that are JSON that can be created via the Builder | ||
| public void testRoundTripSerDe() throws JsonProcessingException { | ||
| String fullJsonRaw = |
There was a problem hiding this comment.
As most of the complex fields (all of the classes we define, such as Schema etc) are serialized and deserialized via the existing *Parser, I only put one test where the whole JSON is defined as a raw string).
Formatting it well across lines was somewhat difficult, as the assertRoundTripSerializesEquallyFrom method requires that the JSON matches letter for letter, so it can't have newlines.
I can split this out with newlines and then replace them if we'd like though, or even read from an external file if need be.
| // The same JSON but using existing parsers for clarity | ||
| String fullJson = String.format( | ||
| "{\"name\":\"%s\",\"location\":\"%s\",\"schema\":%s,\"spec\":%s," + | ||
| "\"order\":%s,\"properties\":%s,\"stageCreate\":%b}", |
There was a problem hiding this comment.
For strings that continue onto the next line, my editor (with our configuration applied) wanted the next string line to be indented. I can remove that though I started questioning what the style standard is 😅
| Assert.assertTrue("Schemas should be equivalent and have same schema id", | ||
| expected.schema().sameSchema(actual.schema()) && expected.schema().schemaId() == actual.schema().schemaId()); |
There was a problem hiding this comment.
I originally had this as two assertions (one for sameSchema and one for equality checking on schemaId).
If we prefer, I can revert to that.
|
cc @danielcweeks @rdblue @Fokko @nastra and @singhpk234 who has helped contribute to some areas of the REST spec. Kept this one relatively simple given that most of the heavy lifting is done by existing Parsers. |
singhpk234
left a comment
singhpk234
left a comment
There was a problem hiding this comment.
LGTM, Thanks @kbendick !!
core/src/test/java/org/apache/iceberg/rest/requests/TestCreateTableRequest.java
Outdated
Show resolved
Hide resolved
kbendick
force-pushed
the
kb-add-create-table-request-tests
branch
from
59eef97 to
46bcb86
Compare
…TableRequest.java Fix added space Co-authored-by: Prashant Singh <35593236+singhpk234@users.noreply.github.com>
|
Thanks, @kbendick! |
Co-authored-by: Prashant Singh <35593236+singhpk234@users.noreply.github.com>
Co-authored-by: Prashant Singh <35593236+singhpk234@users.noreply.github.com>
3 participants
This adds tests for the serialization / deserialization of
CreateTableRequest.This class is already being used quite a bit in other tests, specifically
CatalogTestsandTestRESTCatalog. It also relied pretty heavily on existing serializers inside ofRESTSerializersfor things likeSchemaetc, which simply call out to our existing parsers.Thus, I kept the tests light in terms of combinations of schemas etc as this is a combination of several tested components.