You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/07/13 14:38:06 UTC
[GitHub] [pulsar] poorbarcode opened a new pull request, #16577: [improve] [admin] [PIP-179] Support the admin API to check unknown request parameters
poorbarcode opened a new pull request, #16577:
URL: https://github.com/apache/pulsar/pull/16577
Master Issue: #16135
### Motivation
see #16135
### Modifications
I will complete proposal #16135 with these pull requests( *current pull request is the step-1* ):
1. Add A Jackson component `DynamicSkipUnknownPropertyHandler` that supports dynamically policy of rejecting unknown parameters. Prints warn log when it receives an unknown parameter.
2. Dynamic configuration.
### Documentation
- [ ] `doc-required`
- [x] `doc-not-needed`
- [ ] `doc`
- [ ] `doc-complete`
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] poorbarcode commented on pull request #16577: [improve] [admin] [PIP-179] Support the admin API to check unknown request parameters
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #16577:
URL: https://github.com/apache/pulsar/pull/16577#issuecomment-1186732656
/pulsarbot run-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a diff in pull request #16577: [improve] [admin] [PIP-179] Support the admin API to check unknown request parameters
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #16577:
URL: https://github.com/apache/pulsar/pull/16577#discussion_r922921852
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/DynamicSkipUnknownPropertyHandler.java:
##########
@@ -0,0 +1,62 @@
+/**
+ * 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.pulsar.broker.web;
+
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.JsonDeserializer;
+import com.fasterxml.jackson.databind.deser.DeserializationProblemHandler;
+import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException;
+import java.io.IOException;
+import java.util.Collection;
+import lombok.Getter;
+import lombok.Setter;
+import lombok.extern.slf4j.Slf4j;
+
+@Slf4j
+public class DynamicSkipUnknownPropertyHandler extends DeserializationProblemHandler {
+
+ @Getter
+ @Setter
+ private boolean skipUnknownProperty = true;
+
+ @Override
+ public boolean handleUnknownProperty(DeserializationContext deserializationContext, JsonParser p,
+ JsonDeserializer<?> deserializer, Object beanOrClass,
+ String propertyName) throws IOException {
+ if (skipUnknownProperty){
+ StringBuilder warnLog = new StringBuilder();
+ warnLog.append("Deserialize json to [").append(beanOrClass.getClass().getName())
+ .append("], found unknown property [").append(propertyName).append("] and skipped. ");
+ if (p.isExpectedStartArrayToken()){
+ warnLog.append("The requested value is an array.");
+ } else if (p.isExpectedStartObjectToken()){
+ warnLog.append("The requested value is an object.");
+ } else {
+ warnLog.append("The requested value is [").append(p.getText()).append("].");
+ }
+ log.warn(warnLog.toString());
Review Comment:
I think we should use debug here? Because the skips the unknown property is expected in this case. Users can introduce REST API interceptors, maybe some cases they want to carry more information in the request body but don't want to see warning logs for each request.
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminRestTest.java:
##########
@@ -0,0 +1,95 @@
+/**
+ * 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.pulsar.broker.admin;
+
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+import javax.ws.rs.client.Client;
+import javax.ws.rs.client.ClientBuilder;
+import javax.ws.rs.client.Entity;
+import javax.ws.rs.client.WebTarget;
+import javax.ws.rs.core.MediaType;
+import javax.ws.rs.core.Response;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest;
+import org.apache.pulsar.common.policies.data.ClusterData;
+import org.apache.pulsar.common.policies.data.TenantInfoImpl;
+import org.testng.Assert;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+@Slf4j
+public class AdminRestTest extends MockedPulsarServiceBaseTest {
Review Comment:
Need to provide the test group.
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/DynamicSkipUnknownPropertyHandler.java:
##########
@@ -0,0 +1,62 @@
+/**
+ * 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.pulsar.broker.web;
+
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.JsonDeserializer;
+import com.fasterxml.jackson.databind.deser.DeserializationProblemHandler;
+import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException;
+import java.io.IOException;
+import java.util.Collection;
+import lombok.Getter;
+import lombok.Setter;
+import lombok.extern.slf4j.Slf4j;
+
+@Slf4j
+public class DynamicSkipUnknownPropertyHandler extends DeserializationProblemHandler {
+
+ @Getter
+ @Setter
+ private boolean skipUnknownProperty = true;
+
+ @Override
+ public boolean handleUnknownProperty(DeserializationContext deserializationContext, JsonParser p,
+ JsonDeserializer<?> deserializer, Object beanOrClass,
+ String propertyName) throws IOException {
+ if (skipUnknownProperty){
+ StringBuilder warnLog = new StringBuilder();
+ warnLog.append("Deserialize json to [").append(beanOrClass.getClass().getName())
+ .append("], found unknown property [").append(propertyName).append("] and skipped. ");
+ if (p.isExpectedStartArrayToken()){
+ warnLog.append("The requested value is an array.");
+ } else if (p.isExpectedStartObjectToken()){
+ warnLog.append("The requested value is an object.");
+ } else {
+ warnLog.append("The requested value is [").append(p.getText()).append("].");
+ }
+ log.warn(warnLog.toString());
+ p.skipChildren();
+ return skipUnknownProperty;
+ } else {
+ Collection<Object> propIds = (deserializer == null) ? null : deserializer.getKnownPropertyNames();
Review Comment:
Instead, we should have a detailed error log here? I'm not sure if the Jetty will print the detailed logs, please help check. If Jetty can tell us which property is the unknown property, I think we don't need error logs here.
##########
pulsar-broker-common/src/test/java/org/apache/pulsar/broker/web/DynamicSkipUnknownPropertyHandlerTest.java:
##########
@@ -0,0 +1,125 @@
+/**
+ * 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.pulsar.broker.web;
+
+import com.fasterxml.jackson.databind.DeserializationFeature;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.ObjectReader;
+import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException;
+import lombok.Data;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+public class DynamicSkipUnknownPropertyHandlerTest {
+
+ @Test
+ public void testHandleUnknownProperty() throws Exception{
+ DynamicSkipUnknownPropertyHandler handler = new DynamicSkipUnknownPropertyHandler();
+ handler.setSkipUnknownProperty(true);
+ // Case 1: initial ObjectMapper with "enable feature".
+ ObjectMapper objectMapper = new ObjectMapper();
+ objectMapper.enable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES);
+ objectMapper.addHandler(handler);
+ ObjectReader objectReader = objectMapper.readerFor(TestBean.class);
+ // Assert skip unknown property and logging: objectMapper.
+ String json = "{\"name1\": \"James\",\"nm\":\"Paul\",\"name2\":\"Eric\"}";
+ TestBean testBean = objectMapper.readValue(json, TestBean.class);
+ Assert.assertNull(testBean.getName());
+ Assert.assertEquals(testBean.getName1(), "James");
+ Assert.assertEquals(testBean.getName2(), "Eric");
+ // Assert skip unknown property and logging: objectReader.
+ testBean = objectReader.readValue(json, TestBean.class);
+ Assert.assertNull(testBean.getName());
+ Assert.assertEquals(testBean.getName1(), "James");
+ Assert.assertEquals(testBean.getName2(), "Eric");
+ // Assert failure on unknown property.
+ handler.setSkipUnknownProperty(false);
+ try {
+ objectMapper.readValue(json, TestBean.class);
+ Assert.fail("Expect UnrecognizedPropertyException when set skipUnknownProperty false.");
+ } catch (UnrecognizedPropertyException e){
+
+ }
+ try {
+ objectReader.readValue(json, TestBean.class);
+ Assert.fail("Expect UnrecognizedPropertyException when set skipUnknownProperty false.");
+ } catch (UnrecognizedPropertyException e){
+
+ }
+ // Case 2: initial ObjectMapper with "disabled feature".
+ objectMapper = new ObjectMapper();
+ objectMapper.enable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES);
+ objectMapper.addHandler(handler);
+ objectReader = objectMapper.readerFor(TestBean.class);
+ // Assert failure on unknown property.
+ try {
+ objectMapper.readValue(json, TestBean.class);
+ Assert.fail("Expect UnrecognizedPropertyException when set skipUnknownProperty false.");
+ } catch (UnrecognizedPropertyException e){
+
+ }
+ try {
+ objectReader.readValue(json, TestBean.class);
+ Assert.fail("Expect UnrecognizedPropertyException when set skipUnknownProperty false.");
+ } catch (UnrecognizedPropertyException e){
+
+ }
+ // Assert skip unknown property and logging.
+ handler.setSkipUnknownProperty(true);
+ testBean = objectMapper.readValue(json, TestBean.class);
+ Assert.assertNull(testBean.getName());
+ Assert.assertEquals(testBean.getName1(), "James");
+ Assert.assertEquals(testBean.getName2(), "Eric");
+ testBean = objectReader.readValue(json, TestBean.class);
+ Assert.assertNull(testBean.getName());
+ Assert.assertEquals(testBean.getName1(), "James");
+ Assert.assertEquals(testBean.getName2(), "Eric");
+ // Case 3: unknown property deserialize by object json.
+ json = "{\"name1\": \"James\",\"nm\":{\"name\":\"Paul\",\"age\":18},\"name2\":\"Eric\"}";
+ // Assert skip unknown property and logging.
+ handler.setSkipUnknownProperty(true);
+ testBean = objectMapper.readValue(json, TestBean.class);
+ Assert.assertNull(testBean.getName());
+ Assert.assertEquals(testBean.getName1(), "James");
+ Assert.assertEquals(testBean.getName2(), "Eric");
+ testBean = objectReader.readValue(json, TestBean.class);
+ Assert.assertNull(testBean.getName());
+ Assert.assertEquals(testBean.getName1(), "James");
+ Assert.assertEquals(testBean.getName2(), "Eric");
+ // Case 4: unknown property deserialize by array json.
+ json = "{\"name1\": \"James\",\"nm\":[\"name\",\"Paul\"],\"name2\":\"Eric\"}";
+ // Assert skip unknown property and logging.
+ handler.setSkipUnknownProperty(true);
+ testBean = objectMapper.readValue(json, TestBean.class);
+ Assert.assertNull(testBean.getName());
+ Assert.assertEquals(testBean.getName1(), "James");
+ Assert.assertEquals(testBean.getName2(), "Eric");
+ testBean = objectReader.readValue(json, TestBean.class);
+ Assert.assertNull(testBean.getName());
+ Assert.assertEquals(testBean.getName1(), "James");
+ Assert.assertEquals(testBean.getName2(), "Eric");
+ }
+
+ @Data
+ public static class TestBean {
Review Comment:
```suggestion
private static class TestBean {
```
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/web/WebService.java:
##########
@@ -75,6 +76,10 @@ public class WebService implements AutoCloseable {
private final FilterInitializer filterInitializer;
private JettyStatisticsCollector jettyStatisticsCollector;
+ @Getter
+ private final DynamicSkipUnknownPropertyHandler sharedUnknownPropertyHandler =
Review Comment:
Can be static?
##########
pulsar-broker-common/src/test/java/org/apache/pulsar/broker/web/DynamicSkipUnknownPropertyHandlerTest.java:
##########
@@ -0,0 +1,125 @@
+/**
+ * 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.pulsar.broker.web;
+
+import com.fasterxml.jackson.databind.DeserializationFeature;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.ObjectReader;
+import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException;
+import lombok.Data;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+public class DynamicSkipUnknownPropertyHandlerTest {
Review Comment:
Need to provide the test group.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui merged pull request #16577: [improve] [admin] [PIP-179] Support the admin API to check unknown request parameters
Posted by GitBox <gi...@apache.org>.
codelipenghui merged PR #16577:
URL: https://github.com/apache/pulsar/pull/16577
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] poorbarcode commented on pull request #16577: [improve] [admin] [PIP-179] Support the admin API to check unknown request parameters
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #16577:
URL: https://github.com/apache/pulsar/pull/16577#issuecomment-1183311078
@Jason918 @eolivelli @Technoboy- @mattisonchao Could you take a look?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] RobertIndie commented on a diff in pull request #16577: [improve] [admin] [PIP-179] Support the admin API to check unknown request parameters
Posted by GitBox <gi...@apache.org>.
RobertIndie commented on code in PR #16577:
URL: https://github.com/apache/pulsar/pull/16577#discussion_r927478084
##########
pulsar-broker-common/src/test/java/org/apache/pulsar/broker/web/DynamicSkipUnknownPropertyHandlerTest.java:
##########
@@ -0,0 +1,126 @@
+/**
+ * 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.pulsar.broker.web;
+
+import com.fasterxml.jackson.databind.DeserializationFeature;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.ObjectReader;
+import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException;
+import lombok.Data;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+@Test(groups = "broker-admin")
+public class DynamicSkipUnknownPropertyHandlerTest {
+
+ @Test
+ public void testHandleUnknownProperty() throws Exception{
Review Comment:
Can we split this big test into serval tests according to the case?
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/DynamicSkipUnknownPropertyHandler.java:
##########
@@ -0,0 +1,64 @@
+/**
+ * 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.pulsar.broker.web;
+
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.JsonDeserializer;
+import com.fasterxml.jackson.databind.deser.DeserializationProblemHandler;
+import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException;
+import java.io.IOException;
+import java.util.Collection;
+import lombok.Getter;
+import lombok.Setter;
+import lombok.extern.slf4j.Slf4j;
+
+@Slf4j
+public class DynamicSkipUnknownPropertyHandler extends DeserializationProblemHandler {
+
+ @Getter
+ @Setter
+ private boolean skipUnknownProperty = true;
+
+ @Override
+ public boolean handleUnknownProperty(DeserializationContext deserializationContext, JsonParser p,
+ JsonDeserializer<?> deserializer, Object beanOrClass,
+ String propertyName) throws IOException {
+ if (skipUnknownProperty){
+ if (log.isDebugEnabled()) {
+ StringBuilder warnLog = new StringBuilder();
Review Comment:
Why don't we use the UnrecognizedPropertyException to generate the log?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] poorbarcode commented on a diff in pull request #16577: [improve] [admin] [PIP-179] Support the admin API to check unknown request parameters
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #16577:
URL: https://github.com/apache/pulsar/pull/16577#discussion_r921575093
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/web/WebService.java:
##########
@@ -75,6 +76,9 @@ public class WebService implements AutoCloseable {
private final FilterInitializer filterInitializer;
private JettyStatisticsCollector jettyStatisticsCollector;
+ @Getter
Review Comment:
> Does this need a getter?
In the next PR, I will use it to change the property `skipUnknownProperty` of the shared object `DynamicSkipUnknownPropertyHandler`.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on a diff in pull request #16577: [improve] [admin] [PIP-179] Support the admin API to check unknown request parameters
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #16577:
URL: https://github.com/apache/pulsar/pull/16577#discussion_r928096053
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/NamespacesV2Test.java:
##########
@@ -53,7 +53,7 @@
import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;
-@Test(groups = "broker-admin-v2")
+@Test(groups = "broker-admin")
Review Comment:
Got it, thanks!
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] poorbarcode commented on a diff in pull request #16577: [improve] [admin] [PIP-179] Support the admin API to check unknown request parameters
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #16577:
URL: https://github.com/apache/pulsar/pull/16577#discussion_r927707084
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/DynamicSkipUnknownPropertyHandler.java:
##########
@@ -0,0 +1,64 @@
+/**
+ * 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.pulsar.broker.web;
+
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.JsonDeserializer;
+import com.fasterxml.jackson.databind.deser.DeserializationProblemHandler;
+import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException;
+import java.io.IOException;
+import java.util.Collection;
+import lombok.Getter;
+import lombok.Setter;
+import lombok.extern.slf4j.Slf4j;
+
+@Slf4j
+public class DynamicSkipUnknownPropertyHandler extends DeserializationProblemHandler {
+
+ @Getter
+ @Setter
+ private boolean skipUnknownProperty = true;
+
+ @Override
+ public boolean handleUnknownProperty(DeserializationContext deserializationContext, JsonParser p,
+ JsonDeserializer<?> deserializer, Object beanOrClass,
+ String propertyName) throws IOException {
+ if (skipUnknownProperty){
+ if (log.isDebugEnabled()) {
+ StringBuilder warnLog = new StringBuilder();
Review Comment:
> Why don't we use the UnrecognizedPropertyException to generate the log?
Good idea. Already fixed.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] poorbarcode commented on pull request #16577: [improve] [admin] [PIP-179] Support the admin API to check unknown request parameters
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #16577:
URL: https://github.com/apache/pulsar/pull/16577#issuecomment-1188616693
/pulsarbot run-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] poorbarcode commented on a diff in pull request #16577: [improve] [admin] [PIP-179] Support the admin API to check unknown request parameters
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #16577:
URL: https://github.com/apache/pulsar/pull/16577#discussion_r923997747
##########
pulsar-broker-common/src/test/java/org/apache/pulsar/broker/web/DynamicSkipUnknownPropertyHandlerTest.java:
##########
@@ -0,0 +1,126 @@
+/**
+ * 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.pulsar.broker.web;
+
+import com.fasterxml.jackson.databind.DeserializationFeature;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.ObjectReader;
+import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException;
+import lombok.Data;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+@Test(groups = "broker-admin-v2")
Review Comment:
I changed the group to `broker-admin`, thanks.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] poorbarcode commented on a diff in pull request #16577: [improve] [admin] [PIP-179] Support the admin API to check unknown request parameters
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #16577:
URL: https://github.com/apache/pulsar/pull/16577#discussion_r927709727
##########
pulsar-broker-common/src/test/java/org/apache/pulsar/broker/web/DynamicSkipUnknownPropertyHandlerTest.java:
##########
@@ -0,0 +1,126 @@
+/**
+ * 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.pulsar.broker.web;
+
+import com.fasterxml.jackson.databind.DeserializationFeature;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.ObjectReader;
+import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException;
+import lombok.Data;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+@Test(groups = "broker-admin")
+public class DynamicSkipUnknownPropertyHandlerTest {
+
+ @Test
+ public void testHandleUnknownProperty() throws Exception{
Review Comment:
> Can we split this big test into serval tests according to the case?
No. This method verifies that the feature is turned on, then turned off, and then turned on again. Both state changes work as expected. Split it leads to code redundancy.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] poorbarcode commented on a diff in pull request #16577: [improve] [admin] [PIP-179] Support the admin API to check unknown request parameters
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #16577:
URL: https://github.com/apache/pulsar/pull/16577#discussion_r922941686
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/DynamicSkipUnknownPropertyHandler.java:
##########
@@ -0,0 +1,62 @@
+/**
+ * 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.pulsar.broker.web;
+
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.JsonDeserializer;
+import com.fasterxml.jackson.databind.deser.DeserializationProblemHandler;
+import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException;
+import java.io.IOException;
+import java.util.Collection;
+import lombok.Getter;
+import lombok.Setter;
+import lombok.extern.slf4j.Slf4j;
+
+@Slf4j
+public class DynamicSkipUnknownPropertyHandler extends DeserializationProblemHandler {
Review Comment:
In general, we can directly modify the ObjectMapper features to solve the problem. But the jersey holds a [copy of the Reader(A LRU cache)](https://github.com/javaee/jersey/blob/master/media/json-jackson/src/main/java/org/glassfish/jersey/jackson/internal/jackson/jaxrs/base/ProviderBase.java#L187-L188), cause we have to use DeserializationProblemHandler.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] poorbarcode commented on a diff in pull request #16577: [improve] [admin] [PIP-179] Support the admin API to check unknown request parameters
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #16577:
URL: https://github.com/apache/pulsar/pull/16577#discussion_r922938965
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/DynamicSkipUnknownPropertyHandler.java:
##########
@@ -0,0 +1,62 @@
+/**
+ * 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.pulsar.broker.web;
+
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.JsonDeserializer;
+import com.fasterxml.jackson.databind.deser.DeserializationProblemHandler;
+import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException;
+import java.io.IOException;
+import java.util.Collection;
+import lombok.Getter;
+import lombok.Setter;
+import lombok.extern.slf4j.Slf4j;
+
+@Slf4j
+public class DynamicSkipUnknownPropertyHandler extends DeserializationProblemHandler {
+
+ @Getter
+ @Setter
+ private boolean skipUnknownProperty = true;
+
+ @Override
+ public boolean handleUnknownProperty(DeserializationContext deserializationContext, JsonParser p,
+ JsonDeserializer<?> deserializer, Object beanOrClass,
+ String propertyName) throws IOException {
+ if (skipUnknownProperty){
+ StringBuilder warnLog = new StringBuilder();
+ warnLog.append("Deserialize json to [").append(beanOrClass.getClass().getName())
+ .append("], found unknown property [").append(propertyName).append("] and skipped. ");
+ if (p.isExpectedStartArrayToken()){
+ warnLog.append("The requested value is an array.");
+ } else if (p.isExpectedStartObjectToken()){
+ warnLog.append("The requested value is an object.");
+ } else {
+ warnLog.append("The requested value is [").append(p.getText()).append("].");
+ }
+ log.warn(warnLog.toString());
+ p.skipChildren();
+ return skipUnknownProperty;
+ } else {
+ Collection<Object> propIds = (deserializer == null) ? null : deserializer.getKnownPropertyNames();
Review Comment:
> I'm not sure if the Jetty will print the detailed logs.
There should be no warning logs by default, but there is a detailed explanation in response.
see [JsonMappingExceptionMapper](https://github.com/javaee/jersey/blob/master/media/json-jackson/src/main/java/org/glassfish/jersey/jackson/internal/jackson/jaxrs/base/JsonMappingExceptionMapper.java). *And I will rewrite the response content in the next PR*.
The default implementation of error response looks like this:
```
Unrecognized field "retention_time_in_minutes" (class org.apache.pulsar.common.policies.data.RetentionPolicies), not marked as ignorable (2 known properties: "retentionSizeInMB", "retentionTimeInMinutes"])
at [Source: (org.glassfish.jersey.message.internal.ReaderInterceptorExecutor$UnCloseableInputStream); line: 1, column: 35] (through reference chain: org.apache.pulsar.common.policies.data.RetentionPolicies["retention_time_in_minutes"])
```
I plan to change it to this(Next PR)：
```
Unknown property "retention_time_in_minutes", perhaps you want to use one of these: "retentionSizeInMB", "retentionTimeInMinutes"]
```
> Instead, we should have a detailed error log here? If Jetty can tell us which property is the unknown property, I think we don't need error logs here.
On the above, warn logs should not be needed.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] poorbarcode commented on a diff in pull request #16577: [improve] [admin] [PIP-179] Support the admin API to check unknown request parameters
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #16577:
URL: https://github.com/apache/pulsar/pull/16577#discussion_r922938965
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/DynamicSkipUnknownPropertyHandler.java:
##########
@@ -0,0 +1,62 @@
+/**
+ * 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.pulsar.broker.web;
+
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.JsonDeserializer;
+import com.fasterxml.jackson.databind.deser.DeserializationProblemHandler;
+import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException;
+import java.io.IOException;
+import java.util.Collection;
+import lombok.Getter;
+import lombok.Setter;
+import lombok.extern.slf4j.Slf4j;
+
+@Slf4j
+public class DynamicSkipUnknownPropertyHandler extends DeserializationProblemHandler {
+
+ @Getter
+ @Setter
+ private boolean skipUnknownProperty = true;
+
+ @Override
+ public boolean handleUnknownProperty(DeserializationContext deserializationContext, JsonParser p,
+ JsonDeserializer<?> deserializer, Object beanOrClass,
+ String propertyName) throws IOException {
+ if (skipUnknownProperty){
+ StringBuilder warnLog = new StringBuilder();
+ warnLog.append("Deserialize json to [").append(beanOrClass.getClass().getName())
+ .append("], found unknown property [").append(propertyName).append("] and skipped. ");
+ if (p.isExpectedStartArrayToken()){
+ warnLog.append("The requested value is an array.");
+ } else if (p.isExpectedStartObjectToken()){
+ warnLog.append("The requested value is an object.");
+ } else {
+ warnLog.append("The requested value is [").append(p.getText()).append("].");
+ }
+ log.warn(warnLog.toString());
+ p.skipChildren();
+ return skipUnknownProperty;
+ } else {
+ Collection<Object> propIds = (deserializer == null) ? null : deserializer.getKnownPropertyNames();
Review Comment:
> I'm not sure if the Jetty will print the detailed logs.
There should be no warning logs by default, but there is a detailed explanation in response.
see [JsonMappingExceptionMapper](https://github.com/javaee/jersey/blob/master/media/json-jackson/src/main/java/org/glassfish/jersey/jackson/internal/jackson/jaxrs/base/JsonMappingExceptionMapper.java). *And I will rewrite the response content in the next PR*.
The default implementation of error response looks like this:
```
Unrecognized field "retention_time_in_minutes" (class org.apache.pulsar.common.policies.data.RetentionPolicies), not marked as ignorable (2 known properties: "retentionSizeInMB", "retentionTimeInMinutes"])
at [Source: (org.glassfish.jersey.message.internal.ReaderInterceptorExecutor$UnCloseableInputStream); line: 1, column: 35] (through reference chain: org.apache.pulsar.common.policies.data.RetentionPolicies["retention_time_in_minutes"])
```
I plan to change it to this(Next PR)：
```
Unknown property "retention_time_in_minutes", perhaps you want to use one of these: ["retentionSizeInMB", "retentionTimeInMinutes"]
```
> Instead, we should have a detailed error log here? If Jetty can tell us which property is the unknown property, I think we don't need error logs here.
On the above, warn logs should not be needed.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] mattisonchao commented on a diff in pull request #16577: [improve] [admin] [PIP-179] Support the admin API to check unknown request parameters
Posted by GitBox <gi...@apache.org>.
mattisonchao commented on code in PR #16577:
URL: https://github.com/apache/pulsar/pull/16577#discussion_r928083697
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/NamespacesV2Test.java:
##########
@@ -53,7 +53,7 @@
import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;
-@Test(groups = "broker-admin-v2")
+@Test(groups = "broker-admin")
Review Comment:
Why change this line?
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] poorbarcode commented on a diff in pull request #16577: [improve] [admin] [PIP-179] Support the admin API to check unknown request parameters
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #16577:
URL: https://github.com/apache/pulsar/pull/16577#discussion_r928084968
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/NamespacesV2Test.java:
##########
@@ -53,7 +53,7 @@
import org.testng.annotations.BeforeMethod;
import org.testng.annotations.Test;
-@Test(groups = "broker-admin-v2")
+@Test(groups = "broker-admin")
Review Comment:
> Why change this line?
In https://github.com/apache/pulsar/blob/master/build/run_unit_group.sh, there is no "broker-admin-v2"
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] nahguam commented on a diff in pull request #16577: [improve] [admin] [PIP-179] Support the admin API to check unknown request parameters
Posted by GitBox <gi...@apache.org>.
nahguam commented on code in PR #16577:
URL: https://github.com/apache/pulsar/pull/16577#discussion_r920918052
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/JsonMapperProvider.java:
##########
@@ -27,6 +29,15 @@
public class JsonMapperProvider implements ContextResolver<ObjectMapper> {
private final ObjectMapper mapper = ObjectMapperFactory.create();
+ public JsonMapperProvider(){
Review Comment:
Is this constructor needed?
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/DynamicSkipUnknownPropertyHandler.java:
##########
@@ -0,0 +1,62 @@
+/**
+ * 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.pulsar.broker.web;
+
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.JsonDeserializer;
+import com.fasterxml.jackson.databind.deser.DeserializationProblemHandler;
+import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException;
+import java.io.IOException;
+import java.util.Collection;
+import lombok.Getter;
+import lombok.Setter;
+import lombok.extern.slf4j.Slf4j;
+
+@Slf4j
+public class DynamicSkipUnknownPropertyHandler extends DeserializationProblemHandler {
+
+ @Getter
+ @Setter
+ private boolean skipUnknownProperty = true;
Review Comment:
Does this need to be mutable? I only see it being mutated in the tests.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/web/WebService.java:
##########
@@ -75,6 +76,9 @@ public class WebService implements AutoCloseable {
private final FilterInitializer filterInitializer;
private JettyStatisticsCollector jettyStatisticsCollector;
+ @Getter
Review Comment:
Does this need a getter?
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/web/WebService.java:
##########
@@ -75,6 +76,9 @@ public class WebService implements AutoCloseable {
private final FilterInitializer filterInitializer;
private JettyStatisticsCollector jettyStatisticsCollector;
+ @Getter
+ private DynamicSkipUnknownPropertyHandler sharedJsonMapperProvider = new DynamicSkipUnknownPropertyHandler();
Review Comment:
```suggestion
private final DynamicSkipUnknownPropertyHandler sharedJsonMapperProvider = new DynamicSkipUnknownPropertyHandler();
```
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/DynamicSkipUnknownPropertyHandler.java:
##########
@@ -0,0 +1,62 @@
+/**
+ * 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.pulsar.broker.web;
+
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.JsonDeserializer;
+import com.fasterxml.jackson.databind.deser.DeserializationProblemHandler;
+import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException;
+import java.io.IOException;
+import java.util.Collection;
+import lombok.Getter;
+import lombok.Setter;
+import lombok.extern.slf4j.Slf4j;
+
+@Slf4j
+public class DynamicSkipUnknownPropertyHandler extends DeserializationProblemHandler {
Review Comment:
I didn't know `DeserializationProblemHandler` existed! Thanks for the education :)
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] poorbarcode commented on a diff in pull request #16577: [improve] [admin] [PIP-179] Support the admin API to check unknown request parameters
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #16577:
URL: https://github.com/apache/pulsar/pull/16577#discussion_r921570314
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/DynamicSkipUnknownPropertyHandler.java:
##########
@@ -0,0 +1,62 @@
+/**
+ * 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.pulsar.broker.web;
+
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.JsonDeserializer;
+import com.fasterxml.jackson.databind.deser.DeserializationProblemHandler;
+import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException;
+import java.io.IOException;
+import java.util.Collection;
+import lombok.Getter;
+import lombok.Setter;
+import lombok.extern.slf4j.Slf4j;
+
+@Slf4j
+public class DynamicSkipUnknownPropertyHandler extends DeserializationProblemHandler {
+
+ @Getter
+ @Setter
+ private boolean skipUnknownProperty = true;
Review Comment:
> Does this need to be mutable? I only see it being mutated in the tests.
Yes, in the design of PIP-179 #16135, we will support dynamic updates.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] poorbarcode commented on a diff in pull request #16577: [improve] [admin] [PIP-179] Support the admin API to check unknown request parameters
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #16577:
URL: https://github.com/apache/pulsar/pull/16577#discussion_r921575093
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/web/WebService.java:
##########
@@ -75,6 +76,9 @@ public class WebService implements AutoCloseable {
private final FilterInitializer filterInitializer;
private JettyStatisticsCollector jettyStatisticsCollector;
+ @Getter
Review Comment:
> Does this need a getter?
In the next PR, I will use it to change the property of the shared object `DynamicSkipUnknownPropertyHandler`.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] poorbarcode commented on a diff in pull request #16577: [improve] [admin] [PIP-179] Support the admin API to check unknown request parameters
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #16577:
URL: https://github.com/apache/pulsar/pull/16577#discussion_r921571809
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/JsonMapperProvider.java:
##########
@@ -27,6 +29,15 @@
public class JsonMapperProvider implements ContextResolver<ObjectMapper> {
private final ObjectMapper mapper = ObjectMapperFactory.create();
+ public JsonMapperProvider(){
Review Comment:
> Is this constructor needed?
Yes. It is useful by [HK2](https://github.com/javaee/hk2).
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] poorbarcode commented on a diff in pull request #16577: [improve] [admin] [PIP-179] Support the admin API to check unknown request parameters
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #16577:
URL: https://github.com/apache/pulsar/pull/16577#discussion_r922941686
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/DynamicSkipUnknownPropertyHandler.java:
##########
@@ -0,0 +1,62 @@
+/**
+ * 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.pulsar.broker.web;
+
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.JsonDeserializer;
+import com.fasterxml.jackson.databind.deser.DeserializationProblemHandler;
+import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException;
+import java.io.IOException;
+import java.util.Collection;
+import lombok.Getter;
+import lombok.Setter;
+import lombok.extern.slf4j.Slf4j;
+
+@Slf4j
+public class DynamicSkipUnknownPropertyHandler extends DeserializationProblemHandler {
Review Comment:
In general, we can directly modify the ObjectMapper features to solve the problem. But the jersey holds a [copy of the Reader(A LRU cache)](https://github.com/javaee/jersey/blob/master/media/json-jackson/src/main/java/org/glassfish/jersey/jackson/internal/jackson/jaxrs/base/ProviderBase.java#L187-L188), cause we have to use `DeserializationProblemHandler`.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] poorbarcode commented on pull request #16577: [improve] [admin] [PIP-179] Support the admin API to check unknown request parameters
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #16577:
URL: https://github.com/apache/pulsar/pull/16577#issuecomment-1184900615
> Maybe we add an end-to-end test to test response code 400.
Good suggestion. Thanks, I will do it.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] poorbarcode commented on a diff in pull request #16577: [improve] [admin] [PIP-179] Support the admin API to check unknown request parameters
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #16577:
URL: https://github.com/apache/pulsar/pull/16577#discussion_r922932087
##########
pulsar-broker/src/test/java/org/apache/pulsar/broker/admin/AdminRestTest.java:
##########
@@ -0,0 +1,95 @@
+/**
+ * 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.pulsar.broker.admin;
+
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
+import javax.ws.rs.client.Client;
+import javax.ws.rs.client.ClientBuilder;
+import javax.ws.rs.client.Entity;
+import javax.ws.rs.client.WebTarget;
+import javax.ws.rs.core.MediaType;
+import javax.ws.rs.core.Response;
+import lombok.extern.slf4j.Slf4j;
+import org.apache.pulsar.broker.auth.MockedPulsarServiceBaseTest;
+import org.apache.pulsar.common.policies.data.ClusterData;
+import org.apache.pulsar.common.policies.data.TenantInfoImpl;
+import org.testng.Assert;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.Test;
+
+@Slf4j
+public class AdminRestTest extends MockedPulsarServiceBaseTest {
Review Comment:
Already fixed.
##########
pulsar-broker-common/src/test/java/org/apache/pulsar/broker/web/DynamicSkipUnknownPropertyHandlerTest.java:
##########
@@ -0,0 +1,125 @@
+/**
+ * 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.pulsar.broker.web;
+
+import com.fasterxml.jackson.databind.DeserializationFeature;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.ObjectReader;
+import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException;
+import lombok.Data;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+public class DynamicSkipUnknownPropertyHandlerTest {
Review Comment:
Already fixed.
##########
pulsar-broker-common/src/test/java/org/apache/pulsar/broker/web/DynamicSkipUnknownPropertyHandlerTest.java:
##########
@@ -0,0 +1,125 @@
+/**
+ * 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.pulsar.broker.web;
+
+import com.fasterxml.jackson.databind.DeserializationFeature;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.ObjectReader;
+import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException;
+import lombok.Data;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+public class DynamicSkipUnknownPropertyHandlerTest {
+
+ @Test
+ public void testHandleUnknownProperty() throws Exception{
+ DynamicSkipUnknownPropertyHandler handler = new DynamicSkipUnknownPropertyHandler();
+ handler.setSkipUnknownProperty(true);
+ // Case 1: initial ObjectMapper with "enable feature".
+ ObjectMapper objectMapper = new ObjectMapper();
+ objectMapper.enable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES);
+ objectMapper.addHandler(handler);
+ ObjectReader objectReader = objectMapper.readerFor(TestBean.class);
+ // Assert skip unknown property and logging: objectMapper.
+ String json = "{\"name1\": \"James\",\"nm\":\"Paul\",\"name2\":\"Eric\"}";
+ TestBean testBean = objectMapper.readValue(json, TestBean.class);
+ Assert.assertNull(testBean.getName());
+ Assert.assertEquals(testBean.getName1(), "James");
+ Assert.assertEquals(testBean.getName2(), "Eric");
+ // Assert skip unknown property and logging: objectReader.
+ testBean = objectReader.readValue(json, TestBean.class);
+ Assert.assertNull(testBean.getName());
+ Assert.assertEquals(testBean.getName1(), "James");
+ Assert.assertEquals(testBean.getName2(), "Eric");
+ // Assert failure on unknown property.
+ handler.setSkipUnknownProperty(false);
+ try {
+ objectMapper.readValue(json, TestBean.class);
+ Assert.fail("Expect UnrecognizedPropertyException when set skipUnknownProperty false.");
+ } catch (UnrecognizedPropertyException e){
+
+ }
+ try {
+ objectReader.readValue(json, TestBean.class);
+ Assert.fail("Expect UnrecognizedPropertyException when set skipUnknownProperty false.");
+ } catch (UnrecognizedPropertyException e){
+
+ }
+ // Case 2: initial ObjectMapper with "disabled feature".
+ objectMapper = new ObjectMapper();
+ objectMapper.enable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES);
+ objectMapper.addHandler(handler);
+ objectReader = objectMapper.readerFor(TestBean.class);
+ // Assert failure on unknown property.
+ try {
+ objectMapper.readValue(json, TestBean.class);
+ Assert.fail("Expect UnrecognizedPropertyException when set skipUnknownProperty false.");
+ } catch (UnrecognizedPropertyException e){
+
+ }
+ try {
+ objectReader.readValue(json, TestBean.class);
+ Assert.fail("Expect UnrecognizedPropertyException when set skipUnknownProperty false.");
+ } catch (UnrecognizedPropertyException e){
+
+ }
+ // Assert skip unknown property and logging.
+ handler.setSkipUnknownProperty(true);
+ testBean = objectMapper.readValue(json, TestBean.class);
+ Assert.assertNull(testBean.getName());
+ Assert.assertEquals(testBean.getName1(), "James");
+ Assert.assertEquals(testBean.getName2(), "Eric");
+ testBean = objectReader.readValue(json, TestBean.class);
+ Assert.assertNull(testBean.getName());
+ Assert.assertEquals(testBean.getName1(), "James");
+ Assert.assertEquals(testBean.getName2(), "Eric");
+ // Case 3: unknown property deserialize by object json.
+ json = "{\"name1\": \"James\",\"nm\":{\"name\":\"Paul\",\"age\":18},\"name2\":\"Eric\"}";
+ // Assert skip unknown property and logging.
+ handler.setSkipUnknownProperty(true);
+ testBean = objectMapper.readValue(json, TestBean.class);
+ Assert.assertNull(testBean.getName());
+ Assert.assertEquals(testBean.getName1(), "James");
+ Assert.assertEquals(testBean.getName2(), "Eric");
+ testBean = objectReader.readValue(json, TestBean.class);
+ Assert.assertNull(testBean.getName());
+ Assert.assertEquals(testBean.getName1(), "James");
+ Assert.assertEquals(testBean.getName2(), "Eric");
+ // Case 4: unknown property deserialize by array json.
+ json = "{\"name1\": \"James\",\"nm\":[\"name\",\"Paul\"],\"name2\":\"Eric\"}";
+ // Assert skip unknown property and logging.
+ handler.setSkipUnknownProperty(true);
+ testBean = objectMapper.readValue(json, TestBean.class);
+ Assert.assertNull(testBean.getName());
+ Assert.assertEquals(testBean.getName1(), "James");
+ Assert.assertEquals(testBean.getName2(), "Eric");
+ testBean = objectReader.readValue(json, TestBean.class);
+ Assert.assertNull(testBean.getName());
+ Assert.assertEquals(testBean.getName1(), "James");
+ Assert.assertEquals(testBean.getName2(), "Eric");
+ }
+
+ @Data
+ public static class TestBean {
Review Comment:
Already fixed.
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/DynamicSkipUnknownPropertyHandler.java:
##########
@@ -0,0 +1,62 @@
+/**
+ * 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.pulsar.broker.web;
+
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.JsonDeserializer;
+import com.fasterxml.jackson.databind.deser.DeserializationProblemHandler;
+import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException;
+import java.io.IOException;
+import java.util.Collection;
+import lombok.Getter;
+import lombok.Setter;
+import lombok.extern.slf4j.Slf4j;
+
+@Slf4j
+public class DynamicSkipUnknownPropertyHandler extends DeserializationProblemHandler {
+
+ @Getter
+ @Setter
+ private boolean skipUnknownProperty = true;
+
+ @Override
+ public boolean handleUnknownProperty(DeserializationContext deserializationContext, JsonParser p,
+ JsonDeserializer<?> deserializer, Object beanOrClass,
+ String propertyName) throws IOException {
+ if (skipUnknownProperty){
+ StringBuilder warnLog = new StringBuilder();
+ warnLog.append("Deserialize json to [").append(beanOrClass.getClass().getName())
+ .append("], found unknown property [").append(propertyName).append("] and skipped. ");
+ if (p.isExpectedStartArrayToken()){
+ warnLog.append("The requested value is an array.");
+ } else if (p.isExpectedStartObjectToken()){
+ warnLog.append("The requested value is an object.");
+ } else {
+ warnLog.append("The requested value is [").append(p.getText()).append("].");
+ }
+ log.warn(warnLog.toString());
Review Comment:
> I think we should use debug here?
In discuss, there is [a warn log suggestion](https://lists.apache.org/thread/wp38xyqbk09x91bwrgjj85ddxyxwr09d)
Already fixed.
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/web/WebService.java:
##########
@@ -75,6 +76,10 @@ public class WebService implements AutoCloseable {
private final FilterInitializer filterInitializer;
private JettyStatisticsCollector jettyStatisticsCollector;
+ @Getter
+ private final DynamicSkipUnknownPropertyHandler sharedUnknownPropertyHandler =
Review Comment:
> Can be static?
Good idea. Already fixed.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] poorbarcode commented on a diff in pull request #16577: [improve] [admin] [PIP-179] Support the admin API to check unknown request parameters
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #16577:
URL: https://github.com/apache/pulsar/pull/16577#discussion_r922938965
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/DynamicSkipUnknownPropertyHandler.java:
##########
@@ -0,0 +1,62 @@
+/**
+ * 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.pulsar.broker.web;
+
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.JsonDeserializer;
+import com.fasterxml.jackson.databind.deser.DeserializationProblemHandler;
+import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException;
+import java.io.IOException;
+import java.util.Collection;
+import lombok.Getter;
+import lombok.Setter;
+import lombok.extern.slf4j.Slf4j;
+
+@Slf4j
+public class DynamicSkipUnknownPropertyHandler extends DeserializationProblemHandler {
+
+ @Getter
+ @Setter
+ private boolean skipUnknownProperty = true;
+
+ @Override
+ public boolean handleUnknownProperty(DeserializationContext deserializationContext, JsonParser p,
+ JsonDeserializer<?> deserializer, Object beanOrClass,
+ String propertyName) throws IOException {
+ if (skipUnknownProperty){
+ StringBuilder warnLog = new StringBuilder();
+ warnLog.append("Deserialize json to [").append(beanOrClass.getClass().getName())
+ .append("], found unknown property [").append(propertyName).append("] and skipped. ");
+ if (p.isExpectedStartArrayToken()){
+ warnLog.append("The requested value is an array.");
+ } else if (p.isExpectedStartObjectToken()){
+ warnLog.append("The requested value is an object.");
+ } else {
+ warnLog.append("The requested value is [").append(p.getText()).append("].");
+ }
+ log.warn(warnLog.toString());
+ p.skipChildren();
+ return skipUnknownProperty;
+ } else {
+ Collection<Object> propIds = (deserializer == null) ? null : deserializer.getKnownPropertyNames();
Review Comment:
> I'm not sure if the Jetty will print the detailed logs.
There should be no warning logs by default, but there is a detailed explanation in response.
see [JsonMappingExceptionMapper](https://github.com/javaee/jersey/blob/master/media/json-jackson/src/main/java/org/glassfish/jersey/jackson/internal/jackson/jaxrs/base/JsonMappingExceptionMapper.java). *And I will rewrite the response content in the next PR*.
The default implementation of error response looks like this:
```
Unrecognized field "retention_time_in_minutes" (class org.apache.pulsar.common.policies.data.RetentionPolicies), not marked as ignorable (2 known properties: "retentionSizeInMB", "retentionTimeInMinutes"])
at [Source: (org.glassfish.jersey.message.internal.ReaderInterceptorExecutor$UnCloseableInputStream); line: 1, column: 35] (through reference chain: org.apache.pulsar.common.policies.data.RetentionPolicies["retention_time_in_minutes"])
```
I plan to change it to this：
```
Unknown property "retention_time_in_minutes", perhaps you want to use one of these: "retentionSizeInMB", "retentionTimeInMinutes"]
```
> Instead, we should have a detailed error log here? If Jetty can tell us which property is the unknown property, I think we don't need error logs here.
On the above, warn logs should not be needed.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] poorbarcode commented on a diff in pull request #16577: [improve] [admin] [PIP-179] Support the admin API to check unknown request parameters
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #16577:
URL: https://github.com/apache/pulsar/pull/16577#discussion_r922938965
##########
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/web/DynamicSkipUnknownPropertyHandler.java:
##########
@@ -0,0 +1,62 @@
+/**
+ * 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.pulsar.broker.web;
+
+import com.fasterxml.jackson.core.JsonParser;
+import com.fasterxml.jackson.databind.DeserializationContext;
+import com.fasterxml.jackson.databind.JsonDeserializer;
+import com.fasterxml.jackson.databind.deser.DeserializationProblemHandler;
+import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException;
+import java.io.IOException;
+import java.util.Collection;
+import lombok.Getter;
+import lombok.Setter;
+import lombok.extern.slf4j.Slf4j;
+
+@Slf4j
+public class DynamicSkipUnknownPropertyHandler extends DeserializationProblemHandler {
+
+ @Getter
+ @Setter
+ private boolean skipUnknownProperty = true;
+
+ @Override
+ public boolean handleUnknownProperty(DeserializationContext deserializationContext, JsonParser p,
+ JsonDeserializer<?> deserializer, Object beanOrClass,
+ String propertyName) throws IOException {
+ if (skipUnknownProperty){
+ StringBuilder warnLog = new StringBuilder();
+ warnLog.append("Deserialize json to [").append(beanOrClass.getClass().getName())
+ .append("], found unknown property [").append(propertyName).append("] and skipped. ");
+ if (p.isExpectedStartArrayToken()){
+ warnLog.append("The requested value is an array.");
+ } else if (p.isExpectedStartObjectToken()){
+ warnLog.append("The requested value is an object.");
+ } else {
+ warnLog.append("The requested value is [").append(p.getText()).append("].");
+ }
+ log.warn(warnLog.toString());
+ p.skipChildren();
+ return skipUnknownProperty;
+ } else {
+ Collection<Object> propIds = (deserializer == null) ? null : deserializer.getKnownPropertyNames();
Review Comment:
> I'm not sure if the Jetty will print the detailed logs.
There should be no warning logs by default, but there is a detailed explanation in response.
see [JsonMappingExceptionMapper](https://github.com/javaee/jersey/blob/master/media/json-jackson/src/main/java/org/glassfish/jersey/jackson/internal/jackson/jaxrs/base/JsonMappingExceptionMapper.java). *And I will rewrite the response content in the next PR*.
> Instead, we should have a detailed error log here? If Jetty can tell us which property is the unknown property, I think we don't need error logs here.
On the above, warn logs should not be needed.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] codelipenghui commented on a diff in pull request #16577: [improve] [admin] [PIP-179] Support the admin API to check unknown request parameters
Posted by GitBox <gi...@apache.org>.
codelipenghui commented on code in PR #16577:
URL: https://github.com/apache/pulsar/pull/16577#discussion_r923993375
##########
pulsar-broker-common/src/test/java/org/apache/pulsar/broker/web/DynamicSkipUnknownPropertyHandlerTest.java:
##########
@@ -0,0 +1,126 @@
+/**
+ * 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.pulsar.broker.web;
+
+import com.fasterxml.jackson.databind.DeserializationFeature;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.fasterxml.jackson.databind.ObjectReader;
+import com.fasterxml.jackson.databind.exc.UnrecognizedPropertyException;
+import lombok.Data;
+import org.testng.Assert;
+import org.testng.annotations.Test;
+
+@Test(groups = "broker-admin-v2")
Review Comment:
@poorbarcode Do you know which CI will run the test group `broker-admin-v2`? I can't find it in https://github.com/apache/pulsar/blob/master/build/run_unit_group.sh
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] poorbarcode commented on a diff in pull request #16577: [improve] [admin] [PIP-179] Support the admin API to check unknown request parameters
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on code in PR #16577:
URL: https://github.com/apache/pulsar/pull/16577#discussion_r922940631
##########
pulsar-broker/src/main/java/org/apache/pulsar/broker/web/WebService.java:
##########
@@ -75,6 +76,9 @@ public class WebService implements AutoCloseable {
private final FilterInitializer filterInitializer;
private JettyStatisticsCollector jettyStatisticsCollector;
+ @Getter
+ private DynamicSkipUnknownPropertyHandler sharedJsonMapperProvider = new DynamicSkipUnknownPropertyHandler();
Review Comment:
Already fixed.
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] poorbarcode commented on pull request #16577: [improve] [admin] [PIP-179] Support the admin API to check unknown request parameters
Posted by GitBox <gi...@apache.org>.
poorbarcode commented on PR #16577:
URL: https://github.com/apache/pulsar/pull/16577#issuecomment-1189391556
/pulsarbot run-failure-checks
--
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: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org