You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "OmniaGM (via GitHub)" <gi...@apache.org> on 2023/04/16 22:40:47 UTC
[GitHub] [kafka] OmniaGM opened a new pull request, #13585: [WIP] KAFKA-14737: Move kafka.utils.json to server-common
OmniaGM opened a new pull request, #13585:
URL: https://github.com/apache/kafka/pull/13585
This is a replica of `kafka.utils.json`, we can decide to switch to `org.apache.kafka.server.util.json` everywhere later.
*More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.*
*Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.*
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison merged pull request #13585: KAFKA-14737: Move kafka.utils.json to server-common
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison merged PR #13585:
URL: https://github.com/apache/kafka/pull/13585
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] OmniaGM commented on a diff in pull request #13585: KAFKA-14737: Move kafka.utils.json to server-common
Posted by "OmniaGM (via GitHub)" <gi...@apache.org>.
OmniaGM commented on code in PR #13585:
URL: https://github.com/apache/kafka/pull/13585#discussion_r1261125892
##########
server-common/src/main/java/org/apache/kafka/server/util/json/DecodeJson.java:
##########
@@ -0,0 +1,135 @@
+/*
+ * 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.kafka.server.util.json;
+
+import com.fasterxml.jackson.databind.JsonMappingException;
+import com.fasterxml.jackson.databind.JsonNode;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+public interface DecodeJson<T> {
+ /**
+ * Decode the JSON node provided into an instance of `T`.
+ *
+ * @throws JsonMappingException if `node` cannot be decoded into `T`.
+ */
+ T decode(JsonNode node) throws JsonMappingException;
+
+ static JsonMappingException throwJsonMappingException(String expectedType, JsonNode node) {
+ return new JsonMappingException(null, String.format("Expected `%s` value, received %s", expectedType, node));
+ }
+
+ final class DecodeBoolean implements DecodeJson<Boolean> {
+ @Override
+ public Boolean decode(JsonNode node) throws JsonMappingException {
+ if (node.isBoolean()) {
+ return node.booleanValue();
+ }
+ throw throwJsonMappingException(Boolean.class.getSimpleName(), node);
+ }
+ }
+
+ final class DecodeDouble implements DecodeJson<Double> {
+ @Override
+ public Double decode(JsonNode node) throws JsonMappingException {
+ if (node.isDouble() || node.isLong() || node.isInt()) {
+ return node.doubleValue();
+ }
+ throw throwJsonMappingException(Double.class.getSimpleName(), node);
+ }
+ }
+
+ final class DecodeInteger implements DecodeJson<Integer> {
+ @Override
+ public Integer decode(JsonNode node) throws JsonMappingException {
+ if (node.isInt()) {
+ return node.intValue();
+ }
+ throw throwJsonMappingException(Integer.class.getSimpleName(), node);
+ }
+ }
+
+ final class DecodeLong implements DecodeJson<Long> {
+ @Override
+ public Long decode(JsonNode node) throws JsonMappingException {
+ if (node.isLong() || node.isInt()) {
+ return node.longValue();
+ }
+ throw throwJsonMappingException(Long.class.getSimpleName(), node);
+ }
+ }
+
+ final class DecodeString implements DecodeJson<String> {
+ @Override
+ public String decode(JsonNode node) throws JsonMappingException {
+ if (node.isTextual()) {
+ return node.textValue();
+ }
+ throw throwJsonMappingException(String.class.getSimpleName(), node);
+ }
+ }
+
+ static <E> DecodeJson<Optional<E>> decodeOptional(DecodeJson<E> decodeJson) {
+ return node -> {
+ if (node.isNull()) return Optional.empty();
+ return Optional.of(decodeJson.decode(node));
+ };
+ }
+
+ static <E> DecodeJson<List<E>> decodeList(DecodeJson<E> decodeJson) {
+ return node -> {
+ if (node.isArray()) {
+ List<E> result = new ArrayList<>();
+ Iterator<JsonNode> elements = node.elements();
+ while (elements.hasNext()) {
+ try {
+ result.add(decodeJson.decode(elements.next()));
+ } catch (JsonMappingException e) {
+ throw e;
Review Comment:
Probably not, I was trying to stick to the original interface (which doesn't have any exception in the signature) as much as I could in case we switched everything to use this new class instead. I will remove `catch` and make `decodeList` throw `JsonMappingException` instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] mimaison commented on a diff in pull request #13585: KAFKA-14737: Move kafka.utils.json to server-common
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on code in PR #13585:
URL: https://github.com/apache/kafka/pull/13585#discussion_r1261096752
##########
server-common/src/main/java/org/apache/kafka/server/util/json/DecodeJson.java:
##########
@@ -0,0 +1,135 @@
+/*
+ * 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.kafka.server.util.json;
+
+import com.fasterxml.jackson.databind.JsonMappingException;
+import com.fasterxml.jackson.databind.JsonNode;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+public interface DecodeJson<T> {
+ /**
+ * Decode the JSON node provided into an instance of `T`.
+ *
+ * @throws JsonMappingException if `node` cannot be decoded into `T`.
+ */
+ T decode(JsonNode node) throws JsonMappingException;
+
+ static JsonMappingException throwJsonMappingException(String expectedType, JsonNode node) {
+ return new JsonMappingException(null, String.format("Expected `%s` value, received %s", expectedType, node));
+ }
+
+ final class DecodeBoolean implements DecodeJson<Boolean> {
+ @Override
+ public Boolean decode(JsonNode node) throws JsonMappingException {
+ if (node.isBoolean()) {
+ return node.booleanValue();
+ }
+ throw throwJsonMappingException(Boolean.class.getSimpleName(), node);
+ }
+ }
+
+ final class DecodeDouble implements DecodeJson<Double> {
+ @Override
+ public Double decode(JsonNode node) throws JsonMappingException {
+ if (node.isDouble() || node.isLong() || node.isInt()) {
+ return node.doubleValue();
+ }
+ throw throwJsonMappingException(Double.class.getSimpleName(), node);
+ }
+ }
+
+ final class DecodeInteger implements DecodeJson<Integer> {
+ @Override
+ public Integer decode(JsonNode node) throws JsonMappingException {
+ if (node.isInt()) {
+ return node.intValue();
+ }
+ throw throwJsonMappingException(Integer.class.getSimpleName(), node);
+ }
+ }
+
+ final class DecodeLong implements DecodeJson<Long> {
+ @Override
+ public Long decode(JsonNode node) throws JsonMappingException {
+ if (node.isLong() || node.isInt()) {
+ return node.longValue();
+ }
+ throw throwJsonMappingException(Long.class.getSimpleName(), node);
+ }
+ }
+
+ final class DecodeString implements DecodeJson<String> {
+ @Override
+ public String decode(JsonNode node) throws JsonMappingException {
+ if (node.isTextual()) {
+ return node.textValue();
+ }
+ throw throwJsonMappingException(String.class.getSimpleName(), node);
+ }
+ }
+
+ static <E> DecodeJson<Optional<E>> decodeOptional(DecodeJson<E> decodeJson) {
+ return node -> {
+ if (node.isNull()) return Optional.empty();
+ return Optional.of(decodeJson.decode(node));
+ };
+ }
+
+ static <E> DecodeJson<List<E>> decodeList(DecodeJson<E> decodeJson) {
+ return node -> {
+ if (node.isArray()) {
+ List<E> result = new ArrayList<>();
+ Iterator<JsonNode> elements = node.elements();
+ while (elements.hasNext()) {
+ try {
+ result.add(decodeJson.decode(elements.next()));
+ } catch (JsonMappingException e) {
+ throw e;
Review Comment:
Do we need this `catch` is we rethrow the same exception directly?
Same in `decodeMap()` below
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] OmniaGM commented on a diff in pull request #13585: KAFKA-14737: Move kafka.utils.json to server-common
Posted by "OmniaGM (via GitHub)" <gi...@apache.org>.
OmniaGM commented on code in PR #13585:
URL: https://github.com/apache/kafka/pull/13585#discussion_r1261125892
##########
server-common/src/main/java/org/apache/kafka/server/util/json/DecodeJson.java:
##########
@@ -0,0 +1,135 @@
+/*
+ * 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.kafka.server.util.json;
+
+import com.fasterxml.jackson.databind.JsonMappingException;
+import com.fasterxml.jackson.databind.JsonNode;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+public interface DecodeJson<T> {
+ /**
+ * Decode the JSON node provided into an instance of `T`.
+ *
+ * @throws JsonMappingException if `node` cannot be decoded into `T`.
+ */
+ T decode(JsonNode node) throws JsonMappingException;
+
+ static JsonMappingException throwJsonMappingException(String expectedType, JsonNode node) {
+ return new JsonMappingException(null, String.format("Expected `%s` value, received %s", expectedType, node));
+ }
+
+ final class DecodeBoolean implements DecodeJson<Boolean> {
+ @Override
+ public Boolean decode(JsonNode node) throws JsonMappingException {
+ if (node.isBoolean()) {
+ return node.booleanValue();
+ }
+ throw throwJsonMappingException(Boolean.class.getSimpleName(), node);
+ }
+ }
+
+ final class DecodeDouble implements DecodeJson<Double> {
+ @Override
+ public Double decode(JsonNode node) throws JsonMappingException {
+ if (node.isDouble() || node.isLong() || node.isInt()) {
+ return node.doubleValue();
+ }
+ throw throwJsonMappingException(Double.class.getSimpleName(), node);
+ }
+ }
+
+ final class DecodeInteger implements DecodeJson<Integer> {
+ @Override
+ public Integer decode(JsonNode node) throws JsonMappingException {
+ if (node.isInt()) {
+ return node.intValue();
+ }
+ throw throwJsonMappingException(Integer.class.getSimpleName(), node);
+ }
+ }
+
+ final class DecodeLong implements DecodeJson<Long> {
+ @Override
+ public Long decode(JsonNode node) throws JsonMappingException {
+ if (node.isLong() || node.isInt()) {
+ return node.longValue();
+ }
+ throw throwJsonMappingException(Long.class.getSimpleName(), node);
+ }
+ }
+
+ final class DecodeString implements DecodeJson<String> {
+ @Override
+ public String decode(JsonNode node) throws JsonMappingException {
+ if (node.isTextual()) {
+ return node.textValue();
+ }
+ throw throwJsonMappingException(String.class.getSimpleName(), node);
+ }
+ }
+
+ static <E> DecodeJson<Optional<E>> decodeOptional(DecodeJson<E> decodeJson) {
+ return node -> {
+ if (node.isNull()) return Optional.empty();
+ return Optional.of(decodeJson.decode(node));
+ };
+ }
+
+ static <E> DecodeJson<List<E>> decodeList(DecodeJson<E> decodeJson) {
+ return node -> {
+ if (node.isArray()) {
+ List<E> result = new ArrayList<>();
+ Iterator<JsonNode> elements = node.elements();
+ while (elements.hasNext()) {
+ try {
+ result.add(decodeJson.decode(elements.next()));
+ } catch (JsonMappingException e) {
+ throw e;
Review Comment:
Probably not, I was trying to stick to the original interface (which doesn't have any exception in the signature) as much as I could in case we switched everything to use this new class instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] OmniaGM commented on a diff in pull request #13585: KAFKA-14737: Move kafka.utils.json to server-common
Posted by "OmniaGM (via GitHub)" <gi...@apache.org>.
OmniaGM commented on code in PR #13585:
URL: https://github.com/apache/kafka/pull/13585#discussion_r1262588886
##########
server-common/src/main/java/org/apache/kafka/server/util/json/DecodeJson.java:
##########
@@ -0,0 +1,135 @@
+/*
+ * 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.kafka.server.util.json;
+
+import com.fasterxml.jackson.databind.JsonMappingException;
+import com.fasterxml.jackson.databind.JsonNode;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.Iterator;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+
+public interface DecodeJson<T> {
+ /**
+ * Decode the JSON node provided into an instance of `T`.
+ *
+ * @throws JsonMappingException if `node` cannot be decoded into `T`.
+ */
+ T decode(JsonNode node) throws JsonMappingException;
+
+ static JsonMappingException throwJsonMappingException(String expectedType, JsonNode node) {
+ return new JsonMappingException(null, String.format("Expected `%s` value, received %s", expectedType, node));
+ }
+
+ final class DecodeBoolean implements DecodeJson<Boolean> {
+ @Override
+ public Boolean decode(JsonNode node) throws JsonMappingException {
+ if (node.isBoolean()) {
+ return node.booleanValue();
+ }
+ throw throwJsonMappingException(Boolean.class.getSimpleName(), node);
+ }
+ }
+
+ final class DecodeDouble implements DecodeJson<Double> {
+ @Override
+ public Double decode(JsonNode node) throws JsonMappingException {
+ if (node.isDouble() || node.isLong() || node.isInt()) {
+ return node.doubleValue();
+ }
+ throw throwJsonMappingException(Double.class.getSimpleName(), node);
+ }
+ }
+
+ final class DecodeInteger implements DecodeJson<Integer> {
+ @Override
+ public Integer decode(JsonNode node) throws JsonMappingException {
+ if (node.isInt()) {
+ return node.intValue();
+ }
+ throw throwJsonMappingException(Integer.class.getSimpleName(), node);
+ }
+ }
+
+ final class DecodeLong implements DecodeJson<Long> {
+ @Override
+ public Long decode(JsonNode node) throws JsonMappingException {
+ if (node.isLong() || node.isInt()) {
+ return node.longValue();
+ }
+ throw throwJsonMappingException(Long.class.getSimpleName(), node);
+ }
+ }
+
+ final class DecodeString implements DecodeJson<String> {
+ @Override
+ public String decode(JsonNode node) throws JsonMappingException {
+ if (node.isTextual()) {
+ return node.textValue();
+ }
+ throw throwJsonMappingException(String.class.getSimpleName(), node);
+ }
+ }
+
+ static <E> DecodeJson<Optional<E>> decodeOptional(DecodeJson<E> decodeJson) {
+ return node -> {
+ if (node.isNull()) return Optional.empty();
+ return Optional.of(decodeJson.decode(node));
+ };
+ }
+
+ static <E> DecodeJson<List<E>> decodeList(DecodeJson<E> decodeJson) {
+ return node -> {
+ if (node.isArray()) {
+ List<E> result = new ArrayList<>();
+ Iterator<JsonNode> elements = node.elements();
+ while (elements.hasNext()) {
+ try {
+ result.add(decodeJson.decode(elements.next()));
+ } catch (JsonMappingException e) {
+ throw e;
Review Comment:
I removed the `try/catch`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] OmniaGM commented on pull request #13585: KAFKA-14737: Move kafka.utils.json to server-common
Posted by "OmniaGM (via GitHub)" <gi...@apache.org>.
OmniaGM commented on PR #13585:
URL: https://github.com/apache/kafka/pull/13585#issuecomment-1627718147
> LGTM. Nice work.
>
> Can you please fix the checkstyle import errors?
Thanks for the review. I fixed the import errors
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: jira-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org