ppkarwasz commented on code in PR #3691: URL: https://github.com/apache/logging-log4j2/pull/3691#discussion_r2149663867
########## log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ScriptsPluginTest.java: ########## @@ -0,0 +1,79 @@ +/* + * 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.logging.log4j.core.config; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import org.apache.logging.log4j.core.script.AbstractScript; +import org.junit.jupiter.api.Test; + +class ScriptsPluginTest { + + @Test + public void testCreateScriptsNullInput() { Review Comment: _Nit_: we usually make the test cases package-private methods. ########## log4j-core/src/main/java/org/apache/logging/log4j/core/config/ScriptsPlugin.java: ########## @@ -37,7 +40,18 @@ private ScriptsPlugin() {} */ @PluginFactory public static AbstractScript[] createScripts(@PluginElement("Scripts") final AbstractScript[] scripts) { + if (scripts == null || scripts.length == 0) { + return scripts; + } Review Comment: Returning `null` here will cause an NPE in `AbstractConfiguration`: https://github.com/apache/logging-log4j2/blob/422c385dc9450d4f620a23d84abe2d6a0aa5b9fb/log4j-core/src/main/java/org/apache/logging/log4j/core/config/AbstractConfiguration.java#L719-L728 Since the `PluginElementVisitor` never returns `null` for arrays, I would rather: 1. Mark this class as `@NullMarked` 2. Throw an exception if the argument is `null`: ```suggestion Objects.requireNonNull(scripts); if (scripts.length == 0) { return scripts; } ``` ########## log4j-core-test/src/test/java/org/apache/logging/log4j/core/script/AbstractScriptTest.java: ########## @@ -0,0 +1,70 @@ +/* + * 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.logging.log4j.core.script; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; + +import org.junit.jupiter.api.Test; + +class AbstractScriptTest { + + @Test + public void testConstructorAndGettersWithExplicitName() { + final String lang = "JavaScript"; + final String text = "text"; + final String name = "script"; + final AbstractScript script = new MockScript(name, lang, text); + + assertEquals(lang, script.getLanguage(), "Language should match"); + assertEquals(text, script.getScriptText(), "Script text should match"); + assertEquals(name, script.getName(), "Name should match the provided name"); + assertEquals(name, script.getId(), "Id should match the provided name"); + } + + @Test + public void testConstructorAndGettersWithImplicitName() { + final String lang = "JavaScript"; + final String text = "text"; + final AbstractScript script = new MockScript(null, lang, text); + + assertEquals(lang, script.getLanguage(), "Language should match"); + assertEquals(text, script.getScriptText(), "Script text should match"); + assertNull(script.getName(), "Name should be null"); + assertEquals(script.toString(), script.getId(), "Id should match its toString()"); + } + + @Test + public void testConstructorAndGettersWithBlankName() { + final String lang = "JavaScript"; + final String text = "text"; + final String name = " "; + final AbstractScript script = new MockScript(name, lang, text); + + assertEquals(lang, script.getLanguage(), "Language should match"); + assertEquals(text, script.getScriptText(), "Script text should match"); + assertEquals(name, script.getName(), "Name should be blank"); + assertEquals(script.toString(), script.getId(), "Id should match its toString()"); Review Comment: Same as above. ########## log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ScriptsPluginTest.java: ########## @@ -0,0 +1,79 @@ +/* + * 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.logging.log4j.core.config; + +import static org.junit.jupiter.api.Assertions.assertArrayEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import org.apache.logging.log4j.core.script.AbstractScript; +import org.junit.jupiter.api.Test; + +class ScriptsPluginTest { + + @Test + public void testCreateScriptsNullInput() { Review Comment: Also, if you implement my suggestion above, this case should throw NPE. ########## log4j-core/src/main/java/org/apache/logging/log4j/core/script/package-info.java: ########## @@ -18,7 +18,7 @@ * Log4j 2 Script support. */ @Export -@Version("2.20.2") +@Version("2.25.0") Review Comment: ```suggestion @Version("2.26.0") ``` Tonight I'll release `2.25.0`, so the next minor version will become `2.26.0`. ########## log4j-core-test/src/test/java/org/apache/logging/log4j/core/script/AbstractScriptTest.java: ########## @@ -0,0 +1,70 @@ +/* + * 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.logging.log4j.core.script; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; + +import org.junit.jupiter.api.Test; + +class AbstractScriptTest { + + @Test + public void testConstructorAndGettersWithExplicitName() { + final String lang = "JavaScript"; + final String text = "text"; + final String name = "script"; + final AbstractScript script = new MockScript(name, lang, text); + + assertEquals(lang, script.getLanguage(), "Language should match"); + assertEquals(text, script.getScriptText(), "Script text should match"); + assertEquals(name, script.getName(), "Name should match the provided name"); + assertEquals(name, script.getId(), "Id should match the provided name"); + } + + @Test + public void testConstructorAndGettersWithImplicitName() { + final String lang = "JavaScript"; + final String text = "text"; + final AbstractScript script = new MockScript(null, lang, text); + + assertEquals(lang, script.getLanguage(), "Language should match"); + assertEquals(text, script.getScriptText(), "Script text should match"); + assertNull(script.getName(), "Name should be null"); + assertEquals(script.toString(), script.getId(), "Id should match its toString()"); Review Comment: I would not test the `toString()` method. ########## log4j-core/src/main/java/org/apache/logging/log4j/core/script/AbstractScript.java: ########## @@ -30,11 +31,13 @@ public abstract class AbstractScript { private final String language; private final String scriptText; private final String name; + private final String id; public AbstractScript(final String name, final String language, final String scriptText) { this.language = language; this.scriptText = scriptText; - this.name = name == null ? this.toString() : name; + this.name = name; + this.id = Strings.isBlank(name) ? this.toString() : name; Review Comment: Although the original code **did** call `toString` in the constructor, calling `toString()` in a constructor can be risky if overridden in a subclass—it might access uninitialized fields and cause a `NullPointerException`. The `toString()` method was used to guarantee the uniqueness of the id, we can do the same with: ```suggestion this.id = Strings.isBlank(name) ? Integer.toHexString(Objects.hashCode(this)) : name; ``` -- 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: notifications-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
