Copilot commented on code in PR #3888: URL: https://github.com/apache/hertzbeat/pull/3888#discussion_r2588137393
########## hertzbeat-manager/src/test/java/org/apache/hertzbeat/manager/component/validator/impl/HostParamValidatorTest.java: ########## @@ -0,0 +1,93 @@ +/* + * 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. + */ Review Comment: This test references a HostParamValidator class that does not exist in the main source code. The only host validator implementation is HostParamValidatorAdapter. Either create the HostParamValidator class or update this test to use HostParamValidatorAdapter instead. ########## hertzbeat-manager/src/test/java/org/apache/hertzbeat/manager/component/validator/impl/HostParamValidatorTest.java: ########## @@ -0,0 +1,93 @@ +/* + * 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.hertzbeat.manager.component.validator.impl; + +import org.apache.hertzbeat.common.entity.manager.Param; +import org.apache.hertzbeat.common.entity.manager.ParamDefine; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class HostParamValidatorTest { + + private HostParamValidator validator; + + @BeforeEach + void setUp() { + validator = new HostParamValidator(); + } + + @Test + void support() { + assertTrue(validator.support("host")); + } + + @Test + void validate_ValidHost() { + ParamDefine paramDefine = new ParamDefine(); + paramDefine.setType("host"); + Param param = new Param(); + param.setParamValue("127.0.0.1"); + + assertDoesNotThrow(() -> validator.validate(paramDefine, param)); + } + + @Test + void validate_ValidDomain() { + ParamDefine paramDefine = new ParamDefine(); + paramDefine.setType("host"); + Param param = new Param(); + param.setParamValue("localhost"); + Review Comment: This multi-line comment appears to be developer notes/thoughts rather than documentation. These exploratory comments should be removed before merging as they add clutter and don't provide value to future maintainers. ```suggestion ``` ########## hertzbeat-manager/src/test/java/org/apache/hertzbeat/manager/component/validator/impl/ArrayParamValidatorTest.java: ########## @@ -0,0 +1,97 @@ +/* + * 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.hertzbeat.manager.component.validator.impl; + +import org.apache.hertzbeat.common.entity.manager.Param; +import org.apache.hertzbeat.common.entity.manager.ParamDefine; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class ArrayParamValidatorTest { + + private ArrayParamValidator validator; + + @BeforeEach + void setUp() { + validator = new ArrayParamValidator(); + } + + @Test + void support() { + assertTrue(validator.support("array")); + } + + @Test + void validate_ValidArray() { + ParamDefine paramDefine = new ParamDefine(); + paramDefine.setType("array"); + Param param = new Param(); + param.setParamValue("val1,val2"); + + assertDoesNotThrow(() -> validator.validate(paramDefine, param)); + } + + @Test + void validate_ValidArrayWithBrackets() { + ParamDefine paramDefine = new ParamDefine(); + paramDefine.setType("array"); + Param param = new Param(); + param.setParamValue("[val1,val2]"); + + validator.validate(paramDefine, param); + assertEquals("val1,val2", param.getParamValue()); + } + + @Test + void validate_EmptyArray() { + ParamDefine paramDefine = new ParamDefine(); + paramDefine.setType("array"); + paramDefine.setField("tags"); + Param param = new Param(); + param.setParamValue(""); // split returns empty array if string is empty? No, split("") returns [""] + // length 1 usually, but let's check implementation. + // Implementation: param.getParamValue().split(",") + // If value is "", split returns array of length 1 containing "". + // Wait, if value is empty string, split returns array with one empty string. + // But the validator checks arrays.length == 0. + // Actually split(",") on empty string returns array length 1 [""] in Java. + // So arrays.length == 0 might not be hit for empty string unless string is + // empty? + // Let's check logic: String[] arrays = param.getParamValue().split(","); + // If paramValue is "a", length is 1. + // If paramValue is "", length is 1. + // So when does length == 0 happen? Only if string is empty and limit is + // non-positive? + // Actually, if the input is just empty string, split returns [""] (length 1). + // If input is ",", split returns ["", ""] (length 2). + // So the check `arrays.length == 0` might be unreachable for standard split + // behavior unless specific cases. + // However, let's test what we expect. + + // Re-reading implementation: + // String[] arrays = param.getParamValue().split(","); + // if (arrays.length == 0) ... + + // If I pass empty string, it won't throw. + // If I pass "val1", it won't throw. Review Comment: This test method contains extensive exploratory comments (lines 70-96) analyzing the behavior of String.split() rather than actual test logic. These developer notes should be removed and replaced with actual test assertions or removed entirely if the test case is not needed. ```suggestion param.setParamValue(""); assertDoesNotThrow(() -> validator.validate(paramDefine, param)); assertEquals("", param.getParamValue()); ``` ########## hertzbeat-manager/src/main/java/org/apache/hertzbeat/manager/component/validator/impl/BooleanParamValidator.java: ########## @@ -0,0 +1,43 @@ +/* + * 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.hertzbeat.manager.component.validator.impl; + +import org.apache.hertzbeat.common.entity.manager.Param; +import org.apache.hertzbeat.common.entity.manager.ParamDefine; +import org.apache.hertzbeat.manager.component.validator.ParamValidator; +import org.springframework.stereotype.Component; + +/** + * Number parameter validator Review Comment: The JavaDoc comment incorrectly states "Number parameter validator" but this is the BooleanParamValidator. The comment should say "Boolean parameter validator". ```suggestion * Boolean parameter validator ``` ########## hertzbeat-manager/src/main/java/org/apache/hertzbeat/manager/service/impl/MonitorServiceImpl.java: ########## @@ -45,30 +42,26 @@ import org.apache.hertzbeat.common.entity.manager.ParamDefine; import org.apache.hertzbeat.common.entity.message.CollectRep; import org.apache.hertzbeat.common.support.event.MonitorDeletedEvent; -import org.apache.hertzbeat.common.util.AesUtil; -import org.apache.hertzbeat.common.util.FileUtil; -import org.apache.hertzbeat.common.util.IntervalExpressionUtil; -import org.apache.hertzbeat.common.util.IpDomainUtil; + import org.apache.hertzbeat.common.util.JexlCheckerUtil; -import org.apache.hertzbeat.common.util.JsonUtil; import org.apache.hertzbeat.common.util.SnowFlakeIdGenerator; import org.apache.hertzbeat.grafana.service.DashboardService; -import org.apache.hertzbeat.manager.config.ManagerSseManager; +import org.apache.hertzbeat.manager.component.validator.ParamValidatorManager; import org.apache.hertzbeat.manager.dao.CollectorDao; import org.apache.hertzbeat.manager.dao.CollectorMonitorBindDao; import org.apache.hertzbeat.manager.dao.LabelDao; import org.apache.hertzbeat.manager.dao.MonitorBindDao; import org.apache.hertzbeat.manager.dao.MonitorDao; import org.apache.hertzbeat.manager.dao.ParamDao; import org.apache.hertzbeat.manager.pojo.dto.AppCount; -import org.apache.hertzbeat.manager.pojo.dto.MetricsInfo; import org.apache.hertzbeat.manager.pojo.dto.MonitorDto; +import org.apache.hertzbeat.manager.pojo.dto.MetricsInfo; Review Comment: Imports are not in alphabetical order. MonitorDto should come after MetricsInfo. Consider reordering to: first import MetricsInfo (line 58), then import MonitorDto (line 57). ```suggestion import org.apache.hertzbeat.manager.pojo.dto.MetricsInfo; import org.apache.hertzbeat.manager.pojo.dto.MonitorDto; ``` ########## hertzbeat-manager/src/main/java/org/apache/hertzbeat/manager/component/validator/impl/ArrayParamValidator.java: ########## @@ -0,0 +1,46 @@ +/* + * 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.hertzbeat.manager.component.validator.impl; + +import org.apache.hertzbeat.common.entity.manager.Param; +import org.apache.hertzbeat.common.entity.manager.ParamDefine; +import org.apache.hertzbeat.manager.component.validator.ParamValidator; +import org.springframework.stereotype.Component; + +/** + * Number parameter validator Review Comment: The JavaDoc comment incorrectly states "Number parameter validator" but this is the ArrayParamValidator. The comment should say "Array parameter validator". ```suggestion * Array parameter validator ``` ########## hertzbeat-manager/src/main/java/org/apache/hertzbeat/manager/component/validator/ParamValidatorManager.java: ########## @@ -0,0 +1,49 @@ +/* + * 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.hertzbeat.manager.component.validator; + +import org.apache.hertzbeat.common.entity.manager.Param; +import org.apache.hertzbeat.common.entity.manager.ParamDefine; +import org.springframework.stereotype.Component; + +import java.util.List; + +/** + * Parameter validator manager + */ +@Component +public class ParamValidatorManager { + + private final List<ParamValidator> validators; + + public ParamValidatorManager(List<ParamValidator> validators) { + this.validators = validators; + } + + public void validate(ParamDefine paramDefine, Param param) { + for (ParamValidator validator : validators) { + if (validator.support(paramDefine.getType())) { + validator.validate(paramDefine, param); + return; + } + } + // If no validator found, maybe throw exception or ignore? + // Original code threw exception for default case. Review Comment: The comment "If no validator found, maybe throw exception or ignore?" should be removed or rewritten. The code clearly throws an exception, so the uncertainty in the comment is misleading. Consider removing this comment or rephrasing to: "No validator found for the given type". ```suggestion // No validator found for the given type. ``` ########## hertzbeat-manager/src/main/java/org/apache/hertzbeat/manager/component/validator/impl/ArrayParamValidator.java: ########## @@ -0,0 +1,46 @@ +/* + * 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.hertzbeat.manager.component.validator.impl; + +import org.apache.hertzbeat.common.entity.manager.Param; +import org.apache.hertzbeat.common.entity.manager.ParamDefine; +import org.apache.hertzbeat.manager.component.validator.ParamValidator; +import org.springframework.stereotype.Component; + +/** + * Number parameter validator + */ +@Component +public class ArrayParamValidator implements ParamValidator { + @Override + public boolean support(String type) { + return "array".equals(type); + } + + @Override + public void validate(ParamDefine paramDefine, Param param) { + String[] arrays = param.getParamValue().split(","); + if (arrays.length == 0) { + throw new IllegalArgumentException("Param field" + paramDefine.getField() + " value " Review Comment: Missing space after "field" in error message. Should be "Param field " instead of "Param field". ```suggestion throw new IllegalArgumentException("Param field " + paramDefine.getField() + " value " ``` -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
