[GitHub] [pulsar] codelipenghui commented on a diff in pull request #16577: [improve] [admin] [PIP-179] Support the admin API to check unknown request parameters

[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