Fix the content type retrieval #7013
Conversation
|
|
|
@benmoriceau let me know when you feel this contribution can be reviewed. |
Hello @marcosmarxm, Thanks for the quick feedback. This review is a draft since I am learning the build system. I will check with @jrhizor if I properly respect the good practices when he reach the office. |
| @@ -116,4 +142,17 @@ private static String redactSensitiveInfo(String requestBody) { | |||
| return requestBody; | |||
| } | |||
|
|
|||
| private static boolean isValidJson(String json) { | |||
There was a problem hiding this comment.
We're using Jackson for JSON manipulation almost everywhere and we have a few helpers for it. Can we use Jsons.tryDeserialize instead?
airbyte-server/build.gradle
Outdated
| @@ -57,6 +57,7 @@ dependencies { | |||
| implementation 'org.glassfish.jersey.media:jersey-media-json-jackson' | |||
| implementation 'org.glassfish.jersey.ext:jersey-bean-validation' | |||
| implementation "org.flywaydb:flyway-core:7.14.0" | |||
| implementation 'org.json:json:20210307' | |||
There was a problem hiding this comment.
See other comment, we can remove this dependency if we use already imported JSON libs.
| final List<String> splitLogs = Lists.newArrayList(logs.split(System.lineSeparator())); | ||
| Assertions.assertThat(splitLogs) | ||
| .extracting((line) -> line.endsWith(expectedLog) && line.contains(expectedLogLevel)) | ||
| .containsOnlyOnce(true); |
There was a problem hiding this comment.
| final List<String> splitLogs = Lists.newArrayList(logs.split(System.lineSeparator())); | |
| Assertions.assertThat(splitLogs) | |
| .extracting((line) -> line.endsWith(expectedLog) && line.contains(expectedLogLevel)) | |
| .containsOnlyOnce(true); | |
| final Stream<String> matchingLines = logs.lines() | |
| .filter(line -> line.endsWith(expectedLog)) | |
| .filter(line -> line.contains(expectedLogLevel)); | |
| assertEquals(1, matchingLines.count()); |
Nitpick: personally easier to read with a count of matches instead of a stream of booleans and checking for true. I'm not against adding assertj in general though.
There was a problem hiding this comment.
I switch it to the filter version but I kept assetJ for the comparison.
| private static final String method = "POST"; | ||
| private static final String remoteAddr = "123.456.789.101"; | ||
| private static final String url = "/api/v1/test"; |
There was a problem hiding this comment.
| private static final String method = "POST"; | |
| private static final String remoteAddr = "123.456.789.101"; | |
| private static final String url = "/api/v1/test"; | |
| private static final String METHOD = "POST"; | |
| private static final String REMOTE_ADDR = "123.456.789.101"; | |
| private static final String URL = "/api/v1/test"; |
and usages ofc
* Fix the content type retrieval * Re-add the original MIME type * Fix CS * Update logic and add a test * Auto format * PR comments
What
This is addressing #5900, the content-type retrieval was not working properly and some binary rows were getting log.
How
Use the MIME type describe in https://www.iana.org/assignments/media-types/media-types.xhtml