Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix ignoring trailing JSON content in various places #12907

Merged
merged 6 commits into from
Jun 28, 2022
Merged

Fix ignoring trailing JSON content in various places #12907

merged 6 commits into from
Jun 28, 2022

Conversation

findepi
Copy link
Member

@findepi findepi commented Jun 20, 2022

Follow-up to #12862 and #10783

@findepi findepi added jdbc Relates to Trino JDBC driver no-release-notes This pull request does not require release notes entry labels Jun 20, 2022
@cla-bot cla-bot bot added the cla-signed label Jun 20, 2022
@findepi findepi changed the title Fix ignoring trailing JSON content Fix ignoring trailing JSON content in various places Jun 20, 2022
@findepi findepi force-pushed the findepi/more-json branch from 17fe4bd to 11f054a Compare June 20, 2022 13:54
@findepi findepi requested a review from martint June 20, 2022 13:55
@findepi findepi force-pushed the findepi/more-json branch from 11f054a to f286a40 Compare June 20, 2022 19:46
@findepi findepi mentioned this pull request Jun 20, 2022
Merged
findepi
findepi commented Jun 20, 2022
View reviewed changes
client/trino-client/src/main/java/io/trino/client/JsonCodec.java
T value = mapper.readerFor(javaType).readValue(parser);
checkArgument(parser.nextToken() == null, "Found characters after the expected end of input");
return value;
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar change in Airlift: airlift/airlift#995

@findepi findepi force-pushed the findepi/more-json branch from f286a40 to 781a2c6 Compare June 22, 2022 08:29
findepi added 5 commits June 24, 2022 12:13
@findepi
Refactor JsonUtils
6606231 
- fix ignoring trailing content in JsonUtils.parseJson(String).
  In 88ced23 `parseJson(Path, Class)`
  overload was fixed
- fix exception messages
- place similar methods together
- unify and fix exception handling
@findepi
Fail on trailing content in Accumulo object metadata JSON
5fe47f8
@findepi
Fix ignoring trailing content in client's JsonCodec
7fbffe6 
Similar to commit a7bc2ba, but for
`trino-client`'s `JsonCodec`.
@findepi
Fail on trailing content in Delta Lake metadata JSON
23b350f
@findepi
Fail on trailing content in Kudu table properties JSON
a4ebed3
@findepi findepi force-pushed the findepi/more-json branch from 22d03c4 to 962545d Compare June 24, 2022 10:13
@findepi
Copy link
Member Author

findepi commented Jun 24, 2022

Reviews welcome.

@findepi findepi requested a review from losipiuk June 28, 2022 09:18
losipiuk
losipiuk reviewed Jun 28, 2022
View reviewed changes
...-delta-lake/src/main/java/io/trino/plugin/deltalake/transactionlog/TransactionLogParser.java
@@ -136,7 +137,7 @@ public static DeltaLakeTransactionLogEntry parseJson(String json)
if (json.endsWith("x")) {
json = json.substring(0, json.length() - 1);
}
return OBJECT_MAPPER.readValue(json, DeltaLakeTransactionLogEntry.class);
return JsonUtils.parseJson(OBJECT_MAPPER, json, DeltaLakeTransactionLogEntry.class);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: static import

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is within local parseJson method (name conflict)

losipiuk
losipiuk reviewed Jun 28, 2022
View reviewed changes
...rino-tpcds/src/main/java/io/trino/plugin/tpcds/statistics/TableStatisticsDataRepository.java Outdated
@@ -83,7 +84,7 @@ private Optional<TableStatisticsData> readStatistics(String resourcePath)
return Optional.empty();
}
try {
return Optional.of(objectMapper.readValue(resource, TableStatisticsData.class));
return Optional.of(JsonUtils.parseJson(objectMapper, resource, TableStatisticsData.class));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: static import

@findepi findepi force-pushed the findepi/more-json branch from 962545d to 5f5b914 Compare June 28, 2022 12:06
@findepi findepi merged commit d809130 into trinodb:master Jun 28, 2022
@findepi findepi deleted the findepi/more-json branch June 28, 2022 14:08
@github-actions github-actions bot added this to the 388 milestone Jun 28, 2022
@colebow colebow mentioned this pull request Jun 28, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@losipiuk losipiuk losipiuk approved these changes

@electrum electrum Awaiting requested review from electrum

@ebyhr ebyhr Awaiting requested review from ebyhr

@hashhar hashhar Awaiting requested review from hashhar

@kasiafi kasiafi Awaiting requested review from kasiafi

@martint martint Awaiting requested review from martint

Assignees
No one assigned
Labels
cla-signed jdbc Relates to Trino JDBC driver no-release-notes This pull request does not require release notes entry
Milestone
388
Development

Successfully merging this pull request may close these issues.
2 participants
@findepi @losipiuk