-
Notifications
You must be signed in to change notification settings - Fork 23
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.
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
Conversation
| .replaceAll("^(?i)ODataServiceFor", "") | ||
| .replaceAll("^(?i)RemoteApiFor", "") | ||
| .replaceAll("^(?i)ApiFor", "") | ||
| .replaceAll("^(?i)Api", "") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Always remove "Api"breaking to users relying on Api
Always keep "Api"breaking to users who have been regenerating with.edmxfile updates.
Don't apply any transformations to mappings that are read fromserviceNameMappings.propertiesI 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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
NamingUtils
Context
NamingUtilscurrently makes unintentional changes to the mappings inserviceNameMappings.propertiesIf the file name is
API MATERIAL DOCUMENT SRV.edmxFirst run:
"API_MATERIAL_DOCUMENT_SRV"(from .edmx filename)"APIMaterialDocumentSrv".replace("Api", ""): No match (looking for "Api", found "API")"APIMATERIALDOCUMENTSRV""APIMATERIALDOCUMENTSRV".toLowerCase()="apimaterialdocumentsrv"Second run (re-processing stored value):
APIMATERIALDOCUMENTSRVfor className and"apimaterialdocumentsrv"for packageName (from mapping)"Apimaterialdocumentsrv".replace("Api", ""): MATCHES! Removes "Api""materialdocumentsrv"❌Even though the className stays consistent across runs, the result is semantically flawed since prefix "Api" is not removed.
Feature scope:
Definition of Done
Error handling created / updated & covered by the tests aboveDocumentation updated