Skip to content

fix: [OData] Name mapping bug in NamingUtils #1026

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rpanackal merged 12 commits into from
Dec 10, 2025
Merged

fix: [OData] Name mapping bug in NamingUtils #1026

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3083a1b
test
rpanackal Dec 4, 2025
f6e02ed
Stop reprocessing of existing name
rpanackal Dec 5, 2025
e3d1f4b
formatting
rpanackal Dec 5, 2025
a31c1ab
Target NamingUtils
rpanackal Dec 5, 2025
f862640
fix and test NamingUtils
rpanackal Dec 5, 2025
3867d20
Revert change in EdmService
rpanackal Dec 5, 2025
ea3bec1
test
rpanackal Dec 5, 2025
c00c711
cleanup
rpanackal Dec 5, 2025
0d0d9c9
cleanup
rpanackal Dec 5, 2025
11c07c0
Better regex
rpanackal Dec 9, 2025
70d85b3
replaceFirst instead of replaceAll
rpanackal Dec 10, 2025
ff611a5
Revert "replaceFirst instead of replaceAll"
rpanackal Dec 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions ...enerator-utility/src/main/java/com/sap/cloud/sdk/datamodel/odata/utility/NamingUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,11 +79,11 @@ public static String serviceNameToBaseJavaClassName( @Nonnull final String servi

formattedName =
formattedName
.replace("ODataServiceFor", "")
.replace("RemoteApiFor", "")
.replace("ApiFor", "")
.replace("Api", "")
.replaceAll("Service$", "");
.replaceAll("^(?i)ODataServiceFor", "")
.replaceAll("^(?i)RemoteApiFor", "")
.replaceAll("^(?i)ApiFor", "")
.replaceAll("^(?i)Api", "")

@newtork newtork Dec 5, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Major)

Wouldn't this match edge-cases like "SapInvoice"?
I would probably rather go with something like "A(pi|PI)" or straight up two lines.

Edit: I realize you changed the replacement from "anywhere in the string" to "only in the beginning". Therefore potentially bringing a breaking change for existing users that rely on "Api" being removed from their service name(?).

Which would be fine if we had a major release or sth.

@rpanackal rpanackal Dec 5, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, It is breaking change.

My rational was, to be consistent across regeneration, we have three options

  1. Always remove "Api"
    • breaking to users relying on Api
  2. Always keep "Api"
    • breaking to users who have been regenerating with .edmx file updates.
  3. Don't apply any transformations to mappings that are read from serviceNameMappings.properties
    • I didn't feel confident about removing this additional transformation as it might be a safety measure (?). But, it would be non-breaking.

What do you think about Option 3?

@rpanackal rpanackal Dec 9, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didnt get your comment the first time. I have updated the regex.

@rpanackal rpanackal Dec 9, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It still is not edge-case free. Eg: "Apiary" would be matched. But this is unavoidable

.replaceAll("(?i)Service$", "");

return formattedName;
}
Expand Down
8 changes: 6 additions & 2 deletions ...ator-utility/src/test/java/com/sap/cloud/sdk/datamodel/odata/utility/NamingUtilsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@ private static Stream<Arguments> getClassTestCases()
Arguments.of("Lower,enumeration", "LowerEnumeration"),
Arguments.of("KeepCamelCase", "KeepCamelCase"),
Arguments.of("handle_snake_case", "HandleSnakeCase"),
Arguments.of("OData Service for Business Partner", "BusinessPartner"));
Arguments.of("OData Service for Business Partner", "BusinessPartner"),
Arguments.of("API MATERIAL DOCUMENT SRV", "MATERIALDOCUMENTSRV"),
Arguments.of("MATERIALDOCUMENTSRV", "MATERIALDOCUMENTSRV"));
}

@ParameterizedTest
Expand All @@ -52,7 +54,9 @@ private static Stream<Arguments> getPackageTestCases()
Arguments.of("Lower,enumeration", "lowerenumeration"),
Arguments.of("KeepCamelCase", "keepcamelcase"),
Arguments.of("handle_snake_case", "handlesnakecase"),
Arguments.of("OData Service for Business Partner", "businesspartner"));
Arguments.of("OData Service for Business Partner", "businesspartner"),
Arguments.of("API MATERIAL DOCUMENT SRV", "materialdocumentsrv"),
Arguments.of("materialdocumentsrv", "materialdocumentsrv"));
}

@ParameterizedTest
Expand Down
4 changes: 3 additions & 1 deletion release_notes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,6 @@

### 🐛 Fixed Issues

-
- Fix unintended modification of `serviceNameMappings.properties` during OData service regeneration altering stored mappings.
Additionally, service name cleanup is now case-insensitive for consistency.