-
Notifications
You must be signed in to change notification settings - Fork 31
feat(rt): replace GSON based JSON serde with KMP compatible impl #477
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
5efac86
6ea21d9
e9c599c
289b089
4da6a6d
adb40c6
cd1f752
dbc4a10
5201698
ab208a7
656a757
7131dd1
74a13f4
336074c
4b88f50
2838994
c111e64
ee50ae0
8ba6147
e27566c
377037b
7d76fcf
bffb4a4
c2fdf90
9270970
b67e0ba
2519012
bb966a6
fb0cbff
f780452
7892ddc
22e2ad9
bf711fe
fb2d23f
0889cf9
93eed44
6bb1a3f
54d989a
f45a4a1
b41fac3
be5a6b9
dacca0d
a2f600c
2affe95
78e8be7
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 |
|---|---|---|
|
|
@@ -19,7 +19,7 @@ kotlin { | |
| } | ||
| } | ||
|
|
||
| subprojects { | ||
| allprojects { | ||
| kotlin { | ||
| sourceSets { | ||
| commonTest { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,178 @@ | ||
| How to run JSONTestSuite against serde-json deserialize | ||
| ======================================================== | ||
|
|
||
| When making changes to the lexer it is a good idea to run the | ||
| changes against the [JSONTestSuite](https://github.com/nst/JSONTestSuite) and manually examine the test results. | ||
|
|
||
| ### How to setup the JSONTestSuite | ||
|
Comment on lines
+1
to
+7
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. Question: Why not just add an integration test that uses JSONTestSuite directly into the code?
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. JSONTestSuite is a bit interesting/onerous to setup and use. It does not lend itself to this kind of integration and I see it as an operational cost not worth paying.
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. In this line of thinking, can this code as-is serve as a general purpose JSON parser? If so perhaps it would make sense to break out as a separate dependency at some point in the future such that JSONTestSuite could run against it as the others they have in their repo. Obviously not something for this PR.
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. Unless we are planning to support it as a general purpose parser I would think it probably isn't in our interest (or anyone else) to do that. Perhaps we fork it and set it up so that it's easier to run...
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. The "good idea to run the changes against JSONTestSuite" sounds like best intentions to me, is there a way we can mechanize it? When I was playing around with the testing, I just copy/pasted all of the parser scenarios from their input files into a (admittedly massive) test class. |
||
|
|
||
| 1. Clone the [JSONTestSuite](https://github.com/nst/JSONTestSuite) repository. | ||
| 2. In `JSONTestSuite/parsers`, create a new Gradle JVM application project named `test_smithy_kotlin`. | ||
| 3. Add the following `build.gradle.kts` file | ||
|
|
||
| ```kotlin | ||
| plugins { | ||
| kotlin("jvm") version "1.5.30" | ||
| application | ||
| id("com.github.johnrengelman.shadow") version "7.0.0" | ||
| } | ||
|
|
||
| application { | ||
| mainClass.set("aws.smithy.kotlin.jsontest.MainKt") | ||
| } | ||
|
|
||
| allprojects { | ||
| repositories { | ||
| mavenLocal() | ||
| mavenCentral() | ||
| } | ||
| } | ||
|
|
||
|
|
||
| // NOTE: set to whatever locally published version you are working on | ||
| val smithyKotlinVersion: String = "0.4.1-kmp-json" | ||
|
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. Nit: The versions users have will almost always be something like
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. This readme is targeted towards developers (i.e. us). Not end users. |
||
| dependencies { | ||
| implementation(kotlin("stdlib")) | ||
| implementation("org.jetbrains.kotlinx:kotlinx-coroutines-core:1.5.0") | ||
|
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. Question: Do we need to include a note about matching the coroutines version used by the rest of the project?
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. I'm not sure that's important, again this is targeted at developers of |
||
| implementation("aws.smithy.kotlin:serde-json:$smithyKotlinVersion") | ||
| implementation("aws.smithy.kotlin:utils:$smithyKotlinVersion") | ||
| } | ||
|
|
||
| tasks.jar { | ||
| manifest { | ||
| attributes["Main-Class"] = "aws.smithy.kotlin.jsontest.MainKt" | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| 4. Add the following code to `src/main/kotlin/Main.kt` with: | ||
|
|
||
| ```kotlin | ||
| package aws.smithy.kotlin.jsontest | ||
|
Comment on lines
+48
to
+51
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. Nit: Source file path and
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. It isn't really important since this is creating an application jar. Also technically it would be in line with the recommended directory structure if |
||
|
|
||
| import kotlinx.coroutines.runBlocking | ||
| import kotlin.system.exitProcess | ||
| import java.io.IOException | ||
| import java.nio.file.Files | ||
| import java.nio.file.Paths | ||
| import aws.smithy.kotlin.runtime.serde.json.JsonToken | ||
| import aws.smithy.kotlin.runtime.serde.json.jsonStreamReader | ||
| import aws.smithy.kotlin.runtime.util.InternalApi | ||
|
|
||
|
|
||
| @OptIn(InternalApi::class) | ||
| suspend fun isValidJson(bytes: ByteArray):Boolean { | ||
| val lexer = jsonStreamReader(bytes) | ||
| println(lexer::class.qualifiedName) | ||
| return try { | ||
| val tokens = mutableListOf<JsonToken>() | ||
| do { | ||
| val token = lexer.nextToken() | ||
| tokens.add(token) | ||
| }while(token != JsonToken.EndDocument) | ||
aajtodd marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| // The test suite includes incomplete objects and arrays (e.g. "[null,") | ||
| // These are completely valid for this parser since it's just a tokenizer | ||
| // and doesn't attempt to make semantic meaning from the input. | ||
| // We'll just pretend to fail to satisfy the test suite | ||
| val pruned = if (tokens.last() == JsonToken.EndDocument) tokens.dropLast(1) else tokens | ||
| if (pruned.first() == JsonToken.BeginArray && pruned.last() != JsonToken.EndArray) { | ||
| return false | ||
| } | ||
| if (pruned.first() == JsonToken.BeginObject && pruned.last() != JsonToken.EndObject) { | ||
| return false | ||
| } | ||
|
|
||
| tokens.isNotEmpty() | ||
| }catch(ex: Exception) { | ||
| println(ex) | ||
| false | ||
| } | ||
| } | ||
|
|
||
| fun main(args: Array<String>): Unit = runBlocking { | ||
| if(args.isEmpty()) { | ||
| println("Usage: java TestJSONParsing file.json") | ||
| exitProcess(2) | ||
| } | ||
|
|
||
| try { | ||
| val data = Files.readAllBytes(Paths.get(args[0])) | ||
| if(isValidJson(data)) { | ||
| println("valid"); | ||
| exitProcess(0); | ||
| } | ||
| println("invalid"); | ||
| exitProcess(1); | ||
| } catch (ex: IOException) { | ||
| println(ex) | ||
| println("not found"); | ||
| exitProcess(2); | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| 5. Compile this program with `./gradlew build`. | ||
| NOTE: Be sure to publish all of `smithy-kotlin` "runtime" to maven local. It is helpful to just choose a unique version | ||
| to be sure that everything is wired up correctly. | ||
| 6. Modify `JSONTestSuite/run_tests.py` so that the `programs` dictionary only contains this one entry: | ||
|
|
||
| ``` | ||
| programs = { | ||
| "SmithyKotlin": | ||
| { | ||
| "url":"", | ||
| "commands":["java" , "-jar", os.path.join(PARSERS_DIR, "test_smithy_kotlin/build/libs/test_smithy_kotlin-all.jar")] | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| 7. Run `run_tests.py` and examine the output with a web browser by opening `JSONTestSuite/results/parsing.html`. | ||
|
|
||
| ### Examining the results | ||
|
|
||
| When looking at `JSONTestSuite/results/parsing.html`, there is a matrix of test cases against their | ||
| results with a legend at the top. | ||
|
|
||
| Any test result marked with blue or light blue is for a test case where correct behavior isn't specified, | ||
| so use your best judgement to decide if it should have succeeded or failed. | ||
|
|
||
| The other colors are bad and should be carefully examined. At time of writing, the following test cases | ||
| succeed when they should fail, and we intentionally left it that way since we're not currently concerned | ||
| about being more lenient in the number parsing: | ||
|
|
||
| ``` | ||
| n_number_-01.json [-01] | ||
| n_number_-2..json [-2.] | ||
| n_number_.2e-3.json [.2e-3] | ||
| n_number_0.3e+.json [0.3e+] | ||
| n_number_0.3e.json [0.3e] | ||
| n_number_0.e1.json [0.e1] | ||
| n_number_0_capital_E+.json [0E+] | ||
| n_number_0_capital_E.json [0E] | ||
| n_number_0e+.json [0e+] | ||
| n_number_0e.json [0e] | ||
| n_number_1.0e+.json [1.0e+] | ||
| n_number_1.0e-.json [1.0e-] | ||
| n_number_1.0e.json [1.0e] | ||
| n_number_2.e+3.json [2.e+3] | ||
| n_number_2.e-3.json [2.e-3] | ||
| n_number_2.e3.json [2.e3] | ||
| n_number_9.e+.json [9.e+] | ||
| n_number_neg_int_starting_with_zero.json [-012] | ||
| n_number_neg_real_without_int_part.json [-.123] | ||
| n_number_real_without_fractional_part.json [1.] | ||
| n_number_starting_with_dot.json [.123] | ||
| n_number_with_leading_zero.json [012] | ||
| ``` | ||
|
|
||
|
|
||
|
|
||
| This test case succeeds with our parser and that's OK since we're | ||
| a token streaming parser (multiple values are allowed): | ||
| ``` | ||
| n_array_just_minus.json [-] | ||
| n_structure_double_array.json [][] | ||
| n_structure_whitespace_formfeed.json [0C] <=> [] | ||
| ``` | ||
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.
Question: Why was this change necessary?
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.
It might not be now. It was because we added code to
serde/commonthat needed it. I'll check if it's still required.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.
It was added for
CharStreamtests (since removed). I think I'll keep it since it allows tests to be added to serde common if needed.