Fix #2882: make naming rules for getters stricter (#5050)

Author: cowtowncoder

release-notes/VERSION

@@ -7,6 +7,8 @@ Versions: 3.x (for earlier see VERSION-2.x)
 
 3.0.0-rc2 (not yet released)
 
+#2882: Tighten accessor naming rules to not allow leading lower-case or
+  non-letter character for getters/setters
 #5003: Extend, improve set of `JsonNode.asXxx()` methods for number types
 #5025: Add support for automatic detection of subtypes (like `@JsonSubTypes`)
   from Java 17 sealed types

src/main/java/tools/jackson/databind/introspect/DefaultAccessorNamingStrategy.java

@@ -27,6 +27,17 @@ public class DefaultAccessorNamingStrategy
      * based on various rules.
      */
     public interface BaseNameValidator {
+        /**
+         * Method called to check whether given base name is acceptable: called
+         * for accessor method name after removing prefix.
+         * NOTE: called even if "prefix" is empy (i.e. nothing really removed)
+         * 
+         * @param firstChar First character of the base name
+         * @param basename Full base name (after removing prefix)
+         * @param offset Length of prefix removed
+         *
+         * @return {@ocode true} if base name is acceptable; {@code false} otherwise
+         */
         public boolean accept(char firstChar, String basename, int offset);
     }
 
@@ -239,16 +250,19 @@ protected boolean _isGroovyMetaClassGetter(AnnotatedMethod am) {
      * <li>Is-getter (for Boolean values): "is"
      *  </li>
      *</ul>
-     * and no additional restrictions on base names accepted (configurable for
-     * limits using {@link BaseNameValidator}), allowing names like
-     * "get_value()" and "getvalue()".
+     * with additional restrictions on base names accepted (configurable for
+     * limits using {@link BaseNameValidator}), preventing names like
+     * "get_value()" and "getvalue()" (unlike Jackson 2.x that allowed these).
      */
     public static class Provider
         extends AccessorNamingStrategy.Provider
         implements java.io.Serializable
     {
         private static final long serialVersionUID = 1L;
 
+        private static final BaseNameValidator DEFAULT_BASE_NAME_VALIDATOR
+            = FirstCharBasedValidator.forFirstNameRule(false, false);
+
         protected final String _setterPrefix;
         protected final String _withPrefix;
 
@@ -259,7 +273,7 @@ public static class Provider
 
         public Provider() {
             this("set", JsonPOJOBuilder.DEFAULT_WITH_PREFIX,
-                    "get", "is", null);
+                    "get", "is", DEFAULT_BASE_NAME_VALIDATOR);
         }
 
         protected Provider(Provider p,
@@ -380,7 +394,8 @@ public Provider withIsGetterPrefix(String prefix) {
         public Provider withFirstCharAcceptance(boolean allowLowerCaseFirstChar,
                 boolean allowNonLetterFirstChar) {
             return withBaseNameValidator(
-                    FirstCharBasedValidator.forFirstNameRule(allowLowerCaseFirstChar, allowNonLetterFirstChar));
+                    FirstCharBasedValidator.forFirstNameRule(allowLowerCaseFirstChar,
+                            allowNonLetterFirstChar));
         }
 
         /**
@@ -409,7 +424,8 @@ public AccessorNamingStrategy forPOJO(MapperConfig<?> config, AnnotatedClass tar
         public AccessorNamingStrategy forBuilder(MapperConfig<?> config,
                 AnnotatedClass builderClass, BeanDescription valueTypeDesc)
         {
-            AnnotationIntrospector ai = config.isAnnotationProcessingEnabled() ? config.getAnnotationIntrospector() : null;
+            AnnotationIntrospector ai = config.isAnnotationProcessingEnabled()
+                    ? config.getAnnotationIntrospector() : null;
             JsonPOJOBuilder.Value builderConfig = (ai == null) ? null : ai.findPOJOBuilderConfig(config, builderClass);
             String mutatorPrefix = (builderConfig == null) ? _withPrefix : builderConfig.withPrefix;
             return new DefaultAccessorNamingStrategy(config, builderClass,
@@ -426,14 +442,17 @@ public AccessorNamingStrategy forRecord(MapperConfig<?> config, AnnotatedClass r
 
     /**
      * Simple implementation of {@link BaseNameValidator} that checks the
-     * first character and nothing else.
+     * first character and nothing else if (and only if!) prefix is empty
+     * (so {@code offset} passed is NOT zero).
      *<p>
      * Instances are to be constructed using method
      * {@link FirstCharBasedValidator#forFirstNameRule}.
      */
     public static class FirstCharBasedValidator
-        implements BaseNameValidator
+        implements BaseNameValidator, java.io.Serializable
     {
+        private static final long serialVersionUID = 3L;
+
         private final boolean _allowLowerCaseFirstChar;
         private final boolean _allowNonLetterFirstChar;
 
@@ -468,6 +487,14 @@ public static BaseNameValidator forFirstNameRule(boolean allowLowerCaseFirstChar
 
         @Override
         public boolean accept(char firstChar, String basename, int offset) {
+            // 27-Mar-2025, tatu: [databind#2882] Need to make sure we don't
+            //   try to validate cases where no prefix is removed (mostly case
+            //   for builders, but also for Record-style plain "getters" and
+            //   "setters".
+            if (offset == 0) {
+                return true;
+            }
+            
             // Ok, so... If UTF-16 letter, then check whether lc allowed
             // (title-case and upper-case both assumed to be acceptable by default)
             if (Character.isLetter(firstChar)) {

src/main/java/tools/jackson/databind/util/BeanUtil.java

@@ -21,6 +21,11 @@ public class BeanUtil
     /**********************************************************************
      */
 
+    /**
+     * @deprecated since 3.0.0-rc2 Use {@link tools.jackson.databind.introspect.DefaultAccessorNamingStrategy}
+     *    instead
+     */
+    @Deprecated // since 3.0.0-rc2
     public static String stdManglePropertyName(final String basename, final int offset)
     {
         final int end = basename.length();

src/test/java/tools/jackson/databind/introspect/AccessorNamingForBuilderTest.java

@@ -64,6 +64,7 @@ public void testAccessorCustomWithMethod() throws Exception
                 .accessorNaming(new DefaultAccessorNamingStrategy.Provider()
                         .withBuilderPrefix("")
                 )
+                .enable(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES)
                 .build();
         ValueClassXY xy = customMapper.readValue(json, ValueClassXY.class);
         assertEquals(29, xy._x);

src/test/java/tools/jackson/databind/introspect/AccessorNamingStrategyTest.java

@@ -113,21 +113,21 @@ public AccessorNamingStrategy forPOJO(MapperConfig<?> config, AnnotatedClass val
     /********************************************************
      */
 
-    private final ObjectMapper MAPPER = JsonMapper.builder()
+    private final ObjectMapper ACCNAMING2800_MAPPER = JsonMapper.builder()
             .accessorNaming(new AccNaming2800Provider())
             .build();
 
     @Test
     public void testGetterNaming() throws Exception
     {
         assertEquals(a2q("{'X':3,'Z':true}"),
-                MAPPER.writeValueAsString(new GetterBean2800_XZ()));
+                ACCNAMING2800_MAPPER.writeValueAsString(new GetterBean2800_XZ()));
     }
 
     @Test
     public void testSetterNaming() throws Exception
     {
-        SetterBean2800_Y result = MAPPER.readValue(a2q("{'Y':42}"), SetterBean2800_Y.class);
+        SetterBean2800_Y result = ACCNAMING2800_MAPPER.readValue(a2q("{'Y':42}"), SetterBean2800_Y.class);
         assertEquals(42, result.yyy);
     }
 
@@ -136,10 +136,10 @@ public void testFieldNaming() throws Exception
     {
         // first serialization
         assertEquals(a2q("{'x':1}"),
-                MAPPER.writeValueAsString(new FieldBean2800_X()));
+                ACCNAMING2800_MAPPER.writeValueAsString(new FieldBean2800_X()));
 
         // then deserialization
-        FieldBean2800_X result = MAPPER.readValue(a2q("{'x':28}"),
+        FieldBean2800_X result = ACCNAMING2800_MAPPER.readValue(a2q("{'x':28}"),
                 FieldBean2800_X.class);
         assertEquals(28, result._x);
         assertEquals(2, result.y);
@@ -217,18 +217,19 @@ public void testBaseAccessorCustomSetter() throws Exception
     public void testFirstLetterConfigs() throws Exception
     {
         final FirstLetterVariesBean input = new FirstLetterVariesBean();
-        final String STD_EXP = a2q("{'4Roses':42,'land':true,'value':31337}");
 
-        // First: vanilla? About anything goes
-        ObjectMapper mapper = newJsonMapper();
-        assertEquals(STD_EXP, mapper.writeValueAsString(input));
+        // First: new (3.0) defaults -- no weird stuff
+        assertEquals(a2q("{'value':31337}"),
+                newJsonMapper().writeValueAsString(input));
 
+        // First: let's configure with "anything goes" (2.x default):
         // also if explicitly configured as default:
-        mapper = JsonMapper.builder()
+        ObjectMapper mapper = JsonMapper.builder()
                 .accessorNaming(new DefaultAccessorNamingStrategy.Provider()
                         .withFirstCharAcceptance(true, true))
                 .build();
-        assertEquals(STD_EXP, mapper.writeValueAsString(input));
+        assertEquals(a2q("{'4Roses':42,'land':true,'value':31337}"),
+                mapper.writeValueAsString(input));
 
         // But we can vary it
         mapper = JsonMapper.builder()
@@ -246,5 +247,7 @@ public void testFirstLetterConfigs() throws Exception
                 .build();
         assertEquals(a2q("{'4Roses':42,'value':31337}"),
                 mapper.writeValueAsString(input));
+
+        
     }
 }

src/test/java/tools/jackson/databind/introspect/BeanNamingTest.java

@@ -3,9 +3,11 @@
 import org.junit.jupiter.api.Test;
 
 import tools.jackson.databind.*;
+import tools.jackson.databind.exc.UnrecognizedPropertyException;
 import tools.jackson.databind.testutil.DatabindTestUtil;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.fail;
 
 // Tests for [databind#653]
 public class BeanNamingTest extends DatabindTestUtil
@@ -22,15 +24,55 @@ public int getA() {
         }
     }
 
+    // [databind#2882]
+    static class Bean2882 {
+        // These should NOT be detected as "getters" due to naming conventions
+        public boolean island() { return true; }
+        public boolean is_bad() { return true; }
+
+        public int get_value() { return -2; }
+        public int getter() { return -3; }
+
+        // This is regular and should be detected
+        public int getX() { return 1; }
+
+        // And bad "setter" too
+        public void setter(int x) {
+            throw new IllegalStateException("Should not get called");
+        }
+    }
+
+    private final ObjectMapper MAPPER = newJsonMapper();
+    
     // 24-Sep-2017, tatu: Used to test for `MapperFeature.USE_STD_BEAN_NAMING`, but with 3.x
     //    that is always enabled.
     @Test
     public void testMultipleLeadingCapitalLetters() throws Exception
     {
-        ObjectMapper mapper = newJsonMapper();
         assertEquals(a2q("{'URL':'http:'}"),
-                mapper.writeValueAsString(new URLBean()));
+                MAPPER.writeValueAsString(new URLBean()));
         assertEquals(a2q("{'a':3}"),
-                mapper.writeValueAsString(new ABean()));
+                MAPPER.writeValueAsString(new ABean()));
+    }
+
+    // [databind#2882]
+    @Test
+    void testBadCasingForGetters() throws Exception
+    {
+        assertEquals(a2q("{'x':1}"),
+                MAPPER.writeValueAsString(new Bean2882()));
+    }
+
+    @Test
+    void testBadCasingForSetters() throws Exception
+    {
+        try {
+            MAPPER.readerFor(Bean2882.class)
+                .with(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES)
+                .readValue(a2q("{'ter':1}"));
+            fail("Should not pass");
+        } catch (UnrecognizedPropertyException e) {
+            verifyException(e, "Unrecognized property \"ter\"");
+        }
     }
 }

src/test/java/tools/jackson/databind/introspect/TestPropertyConflicts.java

@@ -7,6 +7,7 @@
 import tools.jackson.databind.*;
 import tools.jackson.databind.cfg.MapperConfig;
 import tools.jackson.databind.exc.InvalidDefinitionException;
+import tools.jackson.databind.json.JsonMapper;
 import tools.jackson.databind.testutil.DatabindTestUtil;
 
 import static org.junit.jupiter.api.Assertions.*;
@@ -99,8 +100,13 @@ public String getStr() {
     public void testFailWithDupProps() throws Exception
     {
         BeanWithConflict bean = new BeanWithConflict();
+        // Must allow "getx"
+        JsonMapper mapper = JsonMapper.builder()
+                .accessorNaming(new DefaultAccessorNamingStrategy.Provider()
+                        .withFirstCharAcceptance(true, false))
+                .build();
         try {
-            String json = MAPPER.writer().writeValueAsString(bean);
+            String json = mapper.writeValueAsString(bean);
             fail("Should have failed due to conflicting accessor definitions; got JSON = "+json);
         } catch (InvalidDefinitionException e) {
             verifyException(e, "Conflicting getter definitions");