This is an automated email from the ASF dual-hosted git repository. rombert pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/sling-agent-skills.git
commit 0f0b02529fe555165c5ad46d9a7f4c77985cf85e Author: Konrad Windszus <[email protected]> AuthorDate: Sat May 9 11:51:05 2026 +0200 Distinguish between literal and reference property values --- osgi-scr-migrator/scripts/migrate_annotations.py | 218 +++++++++++---------- .../scripts/tests/test_migrate_annotations.py | 118 ++++++++++- 2 files changed, 230 insertions(+), 106 deletions(-) diff --git a/osgi-scr-migrator/scripts/migrate_annotations.py b/osgi-scr-migrator/scripts/migrate_annotations.py index acccd3f..31e9eb6 100755 --- a/osgi-scr-migrator/scripts/migrate_annotations.py +++ b/osgi-scr-migrator/scripts/migrate_annotations.py @@ -41,6 +41,12 @@ class Property: description: Optional[str] = None is_configurable: bool = False + def _strip_quotes(self, val: str) -> str: + """Strip surrounding quotes from a value if present.""" + if val.startswith('"') and val.endswith('"'): + return val[1:-1] + return val + def is_property_type_annotation(self) -> bool: """Check if this property should use a Component Property Type annotation. @@ -77,27 +83,64 @@ class Property: Precondition: is_property_type_annotation() returned True. This method should always succeed if the precondition is met. + Unquotes literal values before using them in annotations. """ + val = self._strip_quotes(self.value) + if self.name == 'service.ranking': - return f'@ServiceRanking({self.value})' + return f'@ServiceRanking({val})' elif self.name == 'service.description': - return f'@ServiceDescription("{self.value}")' + return f'@ServiceDescription("{val}")' elif self.name == 'service.vendor': - return f'@ServiceVendor("{self.value}")' + return f'@ServiceVendor("{val}")' # Should never reach here if precondition is met raise ValueError(f"Unknown property type annotation: {self.name}") def to_component_property(self) -> List[str]: - """Convert to @Component property format.""" + """Convert to @Component property format. + + Constant references are formatted as string concatenation: "name="+CONSTANT + Literal values (with quotes stripped) are kept unquoted within the property string: "name=value" + """ properties = [] type_suffix = f":{self.type.value}" if self.type != PropertyType.STRING else "" + def is_reference(val: str) -> bool: + """Return True when the value is not a valid literal for this property type.""" + val = val.strip() + + if self.type == PropertyType.STRING: + return not val.startswith('"') + + if self.type == PropertyType.BOOLEAN: + return not bool(re.fullmatch(r'true|false', val)) + + if self.type in (PropertyType.INTEGER, PropertyType.LONG): + return not bool(re.fullmatch(r'-?\d+[lL]?', val)) + + if self.type in (PropertyType.FLOAT, PropertyType.DOUBLE): + return not bool(re.fullmatch(r'-?(?:\d+\.\d*|\d*\.\d+|\d+)(?:[fFdD])?', val)) + + return True + if self.values: for val in self.values: - properties.append(f'"{self.name}{type_suffix}={val}"') + if is_reference(val): + # Reference - use string concatenation: "name="+CONSTANT + properties.append(f'"{self.name}{type_suffix}="+{val}') + else: + # Literal value - strip quotes and keep unquoted within property string + stripped_val = self._strip_quotes(val) + properties.append(f'"{self.name}{type_suffix}={stripped_val}"') elif self.value is not None: - properties.append(f'"{self.name}{type_suffix}={self.value}"') + if is_reference(self.value): + # Reference - use string concatenation: "name="+CONSTANT + properties.append(f'"{self.name}{type_suffix}="+{self.value}') + else: + # Literal value - strip quotes and keep unquoted within property string + stripped_val = self._strip_quotes(self.value) + properties.append(f'"{self.name}{type_suffix}={stripped_val}"') return properties @@ -359,7 +402,10 @@ class AnnotationMigrator: # Parse attributes name_match = re.search(r'name\s*=\s*"([^"]*)"', annotation) - value_match = re.search(r'(?<!bool)(?<!int)(?<!long)(?<!float)(?<!double)value\s*=\s*"([^"]*)"', annotation) + value_match = re.search( + r'(?<!bool)(?<!int)(?<!long)(?<!float)(?<!double)value\s*=\s*("[^"]*"|[A-Za-z_][A-Za-z0-9_.]*)', + annotation + ) values_match = re.search(r'value\s*=\s*\{([^}]+)\}', annotation) bool_value_match = re.search(r'boolValue\s*=\s*(true|false)', annotation) int_value_match = re.search(r'intValue\s*=\s*(-?\d+)', annotation) @@ -406,11 +452,13 @@ class AnnotationMigrator: prop.type = PropertyType.DOUBLE prop.value = double_value_match.group(1) elif values_match: - # Array of values + # Array of values - preserve quotes on string literals values_str = values_match.group(1) - prop.values = [v.strip().strip('"') for v in values_str.split(',')] + prop.values = [v.strip() for v in values_str.split(',')] elif value_match: - prop.value = value_match.group(1) + raw_value = value_match.group(1) + # Keep string literals quoted; keep constant references unquoted. + prop.value = raw_value if label_match: prop.label = label_match.group(1) @@ -447,6 +495,25 @@ class AnnotationMigrator: def _migrate_sling_servlet(self): """Migrate @SlingServlet to @Component with Sling Servlets Annotations.""" + def parse_annotation_values(annotation_text: str, attr_name: str) -> Optional[List[str]]: + """Parse a SlingServlet attribute as a list of raw Java values. + + Values are returned exactly as Java expressions (quoted string literal or constant + reference), so callers can preserve constants without adding quotes. + """ + array_match = re.search(rf'{attr_name}\s*=\s*\{{([^}}]+)\}}', annotation_text) + if array_match: + return [v.strip() for v in array_match.group(1).split(',') if v.strip()] + + single_match = re.search( + rf'{attr_name}\s*=\s*("[^"]+"|[A-Za-z_][A-Za-z0-9_.]*)', + annotation_text + ) + if single_match: + return [single_match.group(1).strip()] + + return None + i = 0 while i < len(self.lines): line = self.lines[i] @@ -469,12 +536,8 @@ class AnnotationMigrator: indent_str = ' ' * indent # Determine if path-based or resource-type-based - paths_match = re.search(r'paths\s*=\s*\{([^}]+)\}', annotation) - path_match = re.search(r'paths\s*=\s*"([^"]+)"', annotation) if not paths_match else None - rt_match = re.search(r'resourceTypes\s*=\s*\{([^}]+)\}', annotation) - rt_single_match = re.search(r'resourceTypes\s*=\s*"([^"]+)"', annotation) if not rt_match else None - # Also match constant references like ActivityManagerImpl.RESOURCE_TYPE - rt_constant_match = re.search(r'resourceTypes\s*=\s*([A-Za-z_][A-Za-z0-9_.]*)', annotation) if not (rt_match or rt_single_match) else None + path_values = parse_annotation_values(annotation, 'paths') + rt_values = parse_annotation_values(annotation, 'resourceTypes') new_annotations = [] @@ -482,86 +545,29 @@ class AnnotationMigrator: new_annotations.append(f'{indent_str}@Component(service = Servlet.class)') # Path-based servlet - if paths_match or path_match: - if paths_match: - paths = [p.strip().strip('"') for p in paths_match.group(1).split(',')] - paths_str = ', '.join(f'"{p}"' for p in paths) + if path_values: + if len(path_values) == 1: + new_annotations.append(f'{indent_str}@SlingServletPaths(value = {path_values[0]})') + else: + paths_str = ', '.join(path_values) new_annotations.append(f'{indent_str}@SlingServletPaths(value = {{{paths_str}}})') - elif path_match: - new_annotations.append(f'{indent_str}@SlingServletPaths(value = "{path_match.group(1)}")') # Resource-type-based servlet - elif rt_match or rt_single_match or rt_constant_match: + elif rt_values: attrs = [] - if rt_match: - # Parse array elements, preserving whether they're string literals or constants - raw_rts = [rt.strip() for rt in rt_match.group(1).split(',')] - formatted_rts = [] - for rt in raw_rts: - if rt.startswith('"') and rt.endswith('"'): - # String literal - keep as is - formatted_rts.append(rt) - else: - # Constant reference - keep without quotes - formatted_rts.append(rt) - - if len(formatted_rts) == 1: - # Single element - check if it needs quotes - if formatted_rts[0].startswith('"'): - attrs.append(f'resourceTypes = {formatted_rts[0]}') - else: - attrs.append(f'resourceTypes = {formatted_rts[0]}') + def append_attr(attr_name: str, values: Optional[List[str]]): + if not values: + return + if len(values) == 1: + attrs.append(f'{attr_name} = {values[0]}') else: - rts_str = ', '.join(formatted_rts) - attrs.append(f'resourceTypes = {{{rts_str}}}') - elif rt_single_match: - attrs.append(f'resourceTypes = "{rt_single_match.group(1)}"') - elif rt_constant_match: - # Constant reference - use as-is without quotes - attrs.append(f'resourceTypes = {rt_constant_match.group(1)}') - - # Selectors - selectors_match = re.search(r'selectors\s*=\s*\{([^}]+)\}', annotation) - selector_match = re.search(r'selectors\s*=\s*"([^"]+)"', annotation) if not selectors_match else None - - if selectors_match: - sels = [s.strip().strip('"') for s in selectors_match.group(1).split(',')] - if len(sels) == 1: - attrs.append(f'selectors = "{sels[0]}"') - else: - sels_str = ', '.join(f'"{s}"' for s in sels) - attrs.append(f'selectors = {{{sels_str}}}') - elif selector_match: - attrs.append(f'selectors = "{selector_match.group(1)}"') - - # Extensions - ext_match = re.search(r'extensions\s*=\s*\{([^}]+)\}', annotation) - ext_single_match = re.search(r'extensions\s*=\s*"([^"]+)"', annotation) if not ext_match else None - - if ext_match: - exts = [e.strip().strip('"') for e in ext_match.group(1).split(',')] - if len(exts) == 1: - attrs.append(f'extensions = "{exts[0]}"') - else: - exts_str = ', '.join(f'"{e}"' for e in exts) - attrs.append(f'extensions = {{{exts_str}}}') - elif ext_single_match: - attrs.append(f'extensions = "{ext_single_match.group(1)}"') - - # Methods - methods_match = re.search(r'methods\s*=\s*\{([^}]+)\}', annotation) - method_match = re.search(r'methods\s*=\s*"([^"]+)"', annotation) if not methods_match else None - - if methods_match: - methods = [m.strip().strip('"') for m in methods_match.group(1).split(',')] - if len(methods) == 1: - attrs.append(f'methods = "{methods[0]}"') - else: - methods_str = ', '.join(f'"{m}"' for m in methods) - attrs.append(f'methods = {{{methods_str}}}') - elif method_match: - attrs.append(f'methods = "{method_match.group(1)}"') + attrs.append(f'{attr_name} = {{{", ".join(values)}}}') + + append_attr('resourceTypes', rt_values) + append_attr('selectors', parse_annotation_values(annotation, 'selectors')) + append_attr('extensions', parse_annotation_values(annotation, 'extensions')) + append_attr('methods', parse_annotation_values(annotation, 'methods')) # Build @SlingServletResourceTypes annotation if len(attrs) == 1: @@ -585,7 +591,7 @@ class AnnotationMigrator: # Add imports self.imports_to_add.add('org.osgi.service.component.annotations.Component') self.imports_to_add.add('javax.servlet.Servlet') - if paths_match or path_match: + if path_values: self.imports_to_add.add('org.apache.sling.servlets.annotations.SlingServletPaths') else: self.imports_to_add.add('org.apache.sling.servlets.annotations.SlingServletResourceTypes') @@ -599,6 +605,14 @@ class AnnotationMigrator: def _migrate_sling_filter(self): """Migrate @SlingFilter to @Component with @SlingServletFilter annotation.""" + def parse_single_annotation_value(annotation_text: str, attr_name: str) -> Optional[str]: + """Parse a SlingFilter single-value attribute as raw Java expression.""" + match = re.search( + rf'{attr_name}\s*=\s*("[^"]+"|-?\d+|[A-Za-z_][A-Za-z0-9_.]*)', + annotation_text + ) + return match.group(1).strip() if match else None + i = 0 while i < len(self.lines): line = self.lines[i] @@ -623,19 +637,21 @@ class AnnotationMigrator: attrs = [] # Scope - scope_match = re.search(r'scope\s*=\s*SlingFilterScope\.(\w+)', annotation) - if scope_match: - attrs.append(f'scope = SlingServletFilterScope.{scope_match.group(1)}') + scope_value = parse_single_annotation_value(annotation, 'scope') + if scope_value: + # Map old enum constants to the new enum class, preserve custom constants as-is. + scope_value = scope_value.replace('SlingFilterScope.', 'SlingServletFilterScope.') + attrs.append(f'scope = {scope_value}') # Order (maps to order parameter in @SlingServletFilter) - order_match = re.search(r'order\s*=\s*(-?\d+)', annotation) - if order_match: - attrs.append(f'order = {order_match.group(1)}') + order_value = parse_single_annotation_value(annotation, 'order') + if order_value: + attrs.append(f'order = {order_value}') # Pattern - pattern_match = re.search(r'pattern\s*=\s*"([^"]+)"', annotation) - if pattern_match: - attrs.append(f'pattern = "{pattern_match.group(1)}"') + pattern_value = parse_single_annotation_value(annotation, 'pattern') + if pattern_value: + attrs.append(f'pattern = {pattern_value}') # Generate annotations new_annotations = [] @@ -666,7 +682,7 @@ class AnnotationMigrator: self.imports_to_add.add('org.osgi.service.component.annotations.Component') self.imports_to_add.add('javax.servlet.Filter') self.imports_to_add.add('org.apache.sling.servlets.annotations.SlingServletFilter') - if scope_match: + if scope_value and ('SlingServletFilterScope.' in scope_value): self.imports_to_add.add('org.apache.sling.servlets.annotations.SlingServletFilterScope') self.changes_made = True diff --git a/osgi-scr-migrator/scripts/tests/test_migrate_annotations.py b/osgi-scr-migrator/scripts/tests/test_migrate_annotations.py index 23f3962..598d845 100644 --- a/osgi-scr-migrator/scripts/tests/test_migrate_annotations.py +++ b/osgi-scr-migrator/scripts/tests/test_migrate_annotations.py @@ -23,7 +23,7 @@ from migrate_annotations import ( class TestPropertyParsing(unittest.TestCase): """Test @Property annotation parsing.""" - def test_simple_string_property(self): + def test_simple_string_literal_property(self): """Test parsing simple string property.""" content = ''' @Property(name = "service.vendor", value = "Apache Software Foundation") @@ -35,7 +35,22 @@ class TestPropertyParsing(unittest.TestCase): self.assertEqual(len(migrator.component_properties), 1) prop = migrator.component_properties[0] self.assertEqual(prop.name, "service.vendor") - self.assertEqual(prop.value, "Apache Software Foundation") + self.assertEqual(prop.value, "\"Apache Software Foundation\"") + self.assertEqual(prop.type, PropertyType.STRING) + + def test_simple_string_reference_property(self): + """Test parsing simple string property.""" + content = ''' +@Property(name = "service.vendor", value = Constants.SERVICE_VENDOR) +''' + stats = MigrationStats() + migrator = AnnotationMigrator(content, Path("test.java"), stats) + migrator._collect_properties() + + self.assertEqual(len(migrator.component_properties), 1) + prop = migrator.component_properties[0] + self.assertEqual(prop.name, "service.vendor") + self.assertEqual(prop.value, "Constants.SERVICE_VENDOR") self.assertEqual(prop.type, PropertyType.STRING) def test_boolean_property(self): @@ -81,7 +96,7 @@ class TestPropertyParsing(unittest.TestCase): prop = migrator.component_properties[0] self.assertEqual(prop.name, "paths") self.assertEqual(len(prop.values), 3) - self.assertEqual(prop.values, ["/path1", "/path2", "/path3"]) + self.assertEqual(prop.values, ["\"/path1\"", "\"/path2\"", "\"/path3\""]) def test_property_with_label(self): """Test parsing property with label and description.""" @@ -105,7 +120,7 @@ class TestPropertyConversion(unittest.TestCase): def test_simple_property_conversion(self): """Test converting simple property to component format.""" - prop = Property(name="service.vendor", value="Apache Software Foundation") + prop = Property(name="service.vendor", value="\"Apache Software Foundation\"") result = prop.to_component_property() self.assertEqual(len(result), 1) @@ -129,7 +144,7 @@ class TestPropertyConversion(unittest.TestCase): def test_array_property_conversion(self): """Test converting array property.""" - prop = Property(name="paths", values=["/path1", "/path2"]) + prop = Property(name="paths", values=["\"/path1\"", "\"/path2\""]) result = prop.to_component_property() self.assertEqual(len(result), 2) @@ -219,6 +234,50 @@ public class MyServlet extends SlingAllMethodsServlet { self.assertNotIn('org.apache.felix.scr.annotations.sling.SlingServlet', new_content) self.assertIn('org.apache.sling.servlets.annotations.SlingServletResourceTypes', new_content) + def test_servlet_with_constant_values_for_all_resource_type_elements(self): + """Test constants are preserved for resourceTypes/selectors/extensions/methods.""" + content = '''package com.example; + +import org.apache.felix.scr.annotations.sling.SlingServlet; + +@SlingServlet( + resourceTypes = MyServlet.RESOURCE_TYPE, + selectors = {MyServlet.SELECTOR_A, "print"}, + extensions = {MyServlet.EXT_JSON, "html"}, + methods = {HttpConstants.METHOD_GET, "POST"} +) +public class MyServlet extends SlingAllMethodsServlet { +} +''' + stats = MigrationStats() + migrator = AnnotationMigrator(content, Path("test.java"), stats) + new_content, changed = migrator.migrate() + + self.assertTrue(changed) + self.assertIn('@SlingServletResourceTypes', new_content) + self.assertIn('resourceTypes = MyServlet.RESOURCE_TYPE', new_content) + self.assertIn('selectors = {MyServlet.SELECTOR_A, "print"}', new_content) + self.assertIn('extensions = {MyServlet.EXT_JSON, "html"}', new_content) + self.assertIn('methods = {HttpConstants.METHOD_GET, "POST"}', new_content) + + def test_path_based_servlet_with_constant_paths(self): + """Test constants are preserved for paths attribute.""" + content = '''package com.example; + +import org.apache.felix.scr.annotations.sling.SlingServlet; + +@SlingServlet(paths = {MyServlet.PATH_A, "/bin/fallback"}) +public class MyServlet extends SlingAllMethodsServlet { +} +''' + stats = MigrationStats() + migrator = AnnotationMigrator(content, Path("test.java"), stats) + new_content, changed = migrator.migrate() + + self.assertTrue(changed) + self.assertIn('@SlingServletPaths(value = {MyServlet.PATH_A, "/bin/fallback"})', new_content) + self.assertIn('org.apache.sling.servlets.annotations.SlingServletPaths', new_content) + def test_sling_filter_migration(self): """Test migrating Sling filter to Sling Servlets Annotations.""" content = '''package com.example; @@ -248,6 +307,55 @@ public class MyFilter implements Filter { self.assertIn('org.apache.sling.servlets.annotations.SlingServletFilter', new_content) self.assertIn('org.apache.sling.servlets.annotations.SlingServletFilterScope', new_content) + def test_sling_filter_with_constant_values(self): + """Test constants are preserved for SlingFilter attributes.""" + content = '''package com.example; + +import org.apache.felix.scr.annotations.sling.SlingFilter; + +@SlingFilter( + scope = MyFilter.SCOPE, + order = MyFilter.ORDER, + pattern = MyFilter.PATTERN +) +public class MyFilter implements Filter { +} +''' + stats = MigrationStats() + migrator = AnnotationMigrator(content, Path("test.java"), stats) + new_content, changed = migrator.migrate() + + self.assertTrue(changed) + self.assertIn('@SlingServletFilter', new_content) + self.assertIn('scope = MyFilter.SCOPE', new_content) + self.assertIn('order = MyFilter.ORDER', new_content) + self.assertIn('pattern = MyFilter.PATTERN', new_content) + + def test_sling_filter_scope_enum_mapping_with_constants(self): + """Test old SlingFilterScope enum constants are mapped to SlingServletFilterScope.""" + content = '''package com.example; + +import org.apache.felix.scr.annotations.sling.SlingFilter; +import org.apache.felix.scr.annotations.sling.SlingFilterScope; + +@SlingFilter( + scope = SlingFilterScope.INCLUDE, + order = FilterOrder.ORDER, + pattern = FilterPatterns.REQUEST +) +public class MyFilter implements Filter { +} +''' + stats = MigrationStats() + migrator = AnnotationMigrator(content, Path("test.java"), stats) + new_content, changed = migrator.migrate() + + self.assertTrue(changed) + self.assertIn('scope = SlingServletFilterScope.INCLUDE', new_content) + self.assertIn('order = FilterOrder.ORDER', new_content) + self.assertIn('pattern = FilterPatterns.REQUEST', new_content) + self.assertIn('org.apache.sling.servlets.annotations.SlingServletFilterScope', new_content) + class TestComponentMigration(unittest.TestCase): """Test @Component migration."""
