OData / ODataConnectedService

A Visual Studio extension for generating client code for OData Services
Other
79 stars 41 forks source link

Dynamics 365 OData metadata contains spaces causing OData generated code to fail compilation #317

Closed pmkevans closed 5 months ago

pmkevans commented 2 years ago

The issue is described here: https://community.dynamics.com/365/financeandoperations/f/dynamics-365-for-finance-and-operations-forum/463165/odata-properties-have-spaces In short, current versions of D365 have spaces in some property names. OData-CLI is generating class and variable names using this information without managing the spaces and thereby generating code that does not compile.

Although the root cause is D365 being noncompliant it would be sensible for ODataConnectedService to have smarter name generation to validate that all names generated are valid C# identifiers.

mikepizzo commented 2 years ago

We could add logic to the class generation (i.e., to replace space with underscore) but that could be ambiguous (i.e., if the store had the same property name in one case with a space and in one case without a space). We could add additional handling for that case, or even just consider it an edge case and "better than what we have now", however, generating the types with properties that don't contain a space is just a small step in what is going to be a much bigger issue, which is the fact that the ODataClient (or any other client using the classes) is going to have to create URLs to query the service, and read/bind responses to the types, and all of the underlying libraries for handling OData Urls and payloads aren't going to be able to deal with property names that contain spaces.

It sounds like there may be a public contribution coming to address this in ConnectedServices, which we are happy to review. However, I am interested in how the classes are being used such that spaces in the property names don't cause other problems on down the line. That is, if it completely unblocks a scenario then I'm happy to take the contribution, but if it just allows generation of classes that can't then be used anywhere then it's not of much value.

sanderson196 commented 1 year ago

As a follow up to the initial Feature request, I have (manually) modified the generated c# code to remove the spaces from the generate method names and where they are used but not removing the spaces from code related to the original Odata Endpoints. For example, ((this._Template Tasks == null)) is converted to ((this._TemplateTasks == null)) but Context.CreateQuery(GetPath("Template tasks")); remains the same.

Once done you get compliable and usable code that appears to still be able to access the OData endpoints properly. I encourage the community to verify my results.

For reference, I use a regex search to find and replace the "offending" c# code (?<!")Template tasks(?!")

magiva commented 1 year ago

has there been any progress on a fix for this ? who owns the issue here? as microsoft introduced the issue and isnt this tool also microsofts ?

sanderson196 commented 1 year ago

Not from Microsoft - they refused to take any action. I was able to browse the community and find out how to (manually) modify the generated code to correct the issues. Takes about an hour of extra work.

Scott Anderson IT Manager

Mobile: +1 9192916265 | Phone: 919.804.6331 Email: @.**@.> 501 Innovative Way | Zebulon, NC 27597 | USA [https://i.imgur.com/15N3tZu.png]https://noelgroup.net/

From: magiva @.> Sent: Friday, November 11, 2022 4:19 AM To: OData/ODataConnectedService @.> Cc: Scott Anderson @.>; Comment @.> Subject: Re: [OData/ODataConnectedService] Dynamics 365 OData metadata contains spaces causing OData generated code to fail compilation (Issue #317)

has there been any progress on a fix for this ? who owns the issue here? as microsoft introduced the issue and isnt this tool also microsofts ?

- Reply to this email directly, view it on GitHubhttps://nam12.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FOData%2FODataConnectedService%2Fissues%2F317%23issuecomment-1311435368&data=05%7C01%7Csanderson%40nomaco.com%7C99eca9c2e2a24cf0ce4f08dac3c5b515%7C52c6e890063f421cb3641664c666f60c%7C0%7C0%7C638037551210639358%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=b94hQofJ5Nol5vwH4%2B2UNXEkm0JLEjTOTlcMZ60Q8i4%3D&reserved=0, or unsubscribehttps://nam12.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FA2A6MRIVD6O7GHRP3RHA2RLWHYFOTANCNFSM6AAAAAAQHI4CJ4&data=05%7C01%7Csanderson%40nomaco.com%7C99eca9c2e2a24cf0ce4f08dac3c5b515%7C52c6e890063f421cb3641664c666f60c%7C0%7C0%7C638037551210639358%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=pUMv55Gcz2NdbxCEAR8IaDQOmKgMLSthPXbwDRRqofU%3D&reserved=0. You are receiving this because you commented.Message ID: @.**@.>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.

magiva commented 1 year ago

do you have any links to the process you used or did you just use regex to replace the space with _ or remove etc ?

sanderson196 commented 1 year ago

Just used Regex for search for and replace. The catch is that you can't change the data between quotes (the reference to Dynamcis) but only change the code function references. Therefore your regex has to exclude items that start and end in "

Scott Anderson IT Manager

Mobile: +1 9192916265 | Phone: 919.804.6331 Email: @.**@.> 501 Innovative Way | Zebulon, NC 27597 | USA [https://i.imgur.com/15N3tZu.png]https://noelgroup.net/

From: magiva @.> Sent: Friday, November 11, 2022 7:53 AM To: OData/ODataConnectedService @.> Cc: Scott Anderson @.>; Comment @.> Subject: Re: [OData/ODataConnectedService] Dynamics 365 OData metadata contains spaces causing OData generated code to fail compilation (Issue #317)

do you have any links to the process you used or did you just use regex to replace the space with _ or remove etc ?

- Reply to this email directly, view it on GitHubhttps://nam12.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FOData%2FODataConnectedService%2Fissues%2F317%23issuecomment-1311664715&data=05%7C01%7Csanderson%40nomaco.com%7Cb72edbbcd6f8423fb93c08dac3e3aee5%7C52c6e890063f421cb3641664c666f60c%7C0%7C0%7C638037679922659393%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=9kqeG5JGcSsoHJ2Aq6MIWOdPJpEqwTXNxukj3zGYsR8%3D&reserved=0, or unsubscribehttps://nam12.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FA2A6MRNPC5IHIZU4LECQHMLWHY6THANCNFSM6AAAAAAQHI4CJ4&data=05%7C01%7Csanderson%40nomaco.com%7Cb72edbbcd6f8423fb93c08dac3e3aee5%7C52c6e890063f421cb3641664c666f60c%7C0%7C0%7C638037679922659393%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=FA%2BGwqZchtbdA7bovv2Akvgg0CN5HuTjXxArTcRCU7Q%3D&reserved=0. You are receiving this because you commented.Message ID: @.**@.>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.

magiva commented 1 year ago

yeah that was our thoughts also, many thanks for the reply, much appreciated

SomeTroglodyte commented 1 year ago

Takes about an hour of extra work

... every time you need to refresh a schema. That in addition to manually fixing the namespace issues preventing use of more than one service per project.

As this does not only affect Dynamics, but all OData using the less common codepoints of the allowed identifier character set, I'd remove the "feature" label. "bug" is more appropriate.

Related to #297, #224 and #242 - there's something basically wrong with the decisions when and how to massage and/or escape external names.

AndreasHassing commented 1 year ago

This has recently been fixed in Dynamics for App 10.0.32 and has been backported to 10.0.30 and 10.0.31.

To find the backport in LCS use this issue #: 753226

magiva commented 1 year ago

thanks for the update, will test soon

dazinator commented 1 year ago

This has recently been fixed in Dynamics for App 10.0.32 and has been backported to 10.0.30 and 10.0.31.

To find the backport in LCS use this issue #: 753226

I am new to LCS. We have a partner account, and an environment with an existing deployment for dev purposes. What does this mean:

To find the backport in LCS use this issue #: 753226

I can't see anywhere to search by issue number exactly - how does this help upgrade a deployment?

AndreasHassing commented 1 year ago

@dazinator see this documentation on how to search the LCS issue database.

See this on how to apply a hotfix to your development environment 😊.

dazinator commented 1 year ago

@AndreasHassing thank you much obliged!

SomeTroglodyte commented 1 year ago

Still broken in public VS extension 1.0.1

AndreasHassing commented 1 year ago

Still broken in public VS extension 1.0.1

This is not an issue with the extension per se, but rather a bug in FnO. As mentioned above, there are hotfixes to solve this on regressed versions.

SomeTroglodyte commented 1 year ago

FnO??? Whatever it is, this is also a plain Visual Studio extension and the official Microsoft answer to consuming OData from .net code. No Dynamics365 involved at all. So any Dynamics365-Specific kludge is irrelevant as long as the basic code fails to map a subset of allowed names. Don't get me wrong - I'm actually kind of glad about it as it saves me a bunch of work and I can tell the other departments "sorry not my fault" :smile_cat:

AndreasHassing commented 1 year ago

FnO??? Whatever it is, this is also a plain Visual Studio extension and the official Microsoft answer to consuming OData from .net code. No Dynamics365 involved at all. So any Dynamics365-Specific kludge is irrelevant as long as the basic code fails to map a subset of allowed names. Don't get me wrong - I'm actually kind of glad about it as it saves me a bunch of work and I can tell the other departments "sorry not my fault" 😸

The bug saves you from doing work? Ok 😀👌.

Anyhow. I imagine the maintainers would prefer a new Issue since this one is primarily for the issue coming from Dynamics 365's OData entity types.

Also, consider adding some more details about what your issues are-- especially considering they are not related to Dynamics.

SomeTroglodyte commented 1 year ago

333 fixes this with >99% probability. Please don't ignore.

about what your issues are

Just dashes in field names in a feed published by another division of my employer. I can read 9 out of 10 of their feeds (which one could describe as custom telemetry to monitor our service quality) no problem, but the one has those dashes, and with the current public version of the vsix the poor compiler gets really confused about all those subtractions where no operators are expected at all... And said PR handles it just fine. Dashes converted to underscores except in the OriginalNameAttribute annotations, just as it should be.

adamsilenko commented 10 months ago

It is still issue in 1.0.4 version... I made a PowerShell script to find & fix generated .cs files. It replace spaces in the names to underline char.

function badNameFix() {
    #declare $entitySetsFolder (with generated .cs files) and $badNameFolder (destination to backup oryginal files)
    $badNamePattern = "^\s*(public virtual global::|private global::|partial )((\w+\.)*\w+(<[^>]+>)?)\s(?'name'\w+( \w+)+)\s*(\([^\)]*\))?;?\s*$"

    # Ensure the destination directory exists
    if (!(Test-Path -Path $badNameFolder)) {
        New-Item -ItemType directory -Path $badNameFolder
    }
    $totalFiles = 0
    $totalNames = 0
    # Iterate over all .cs files in the search directory
    Select-String -Path "$entitySetsFolder\*.cs" -Pattern $badNamePattern | Group-Object -Property Filename | ForEach-Object {
        $fileName = $_.Name 
        Write-Output "File with bad definitions: $fileName"
        $badNames = $_.Group.Matches | ForEach-Object { $_.Groups["name"].Value } | Sort-Object -Descending

        # Read the file content
        $content = Get-Content -Path "$entitySetsFolder\$fileName" -Raw

        foreach($badName in $badNames) {
            $totalNames++
            $corectName = $badName -replace ' ', '_'
            Write-Output "Changing: $badName >> $corectName"
            # Replace the pattern with the replacement string
            $content = $content -creplace "(?<!"")$badName(?!"")", $corectName
        }
        # Move & rename the file to the destination directory
        Move-Item -Path "$entitySetsFolder\$fileName" -Destination "$badNameFolder\$fileName.old"
        # Write the new content to the file
        Set-Content -Path "$entitySetsFolder\$fileName" -Value $content
        $totalFiles++
    }
    Write-Output "Finished. $totalNames names was changed in $totalFiles files."
}