Skip to content
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

Jump to bottom

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

Closed
pmkevans opened this issue Sep 8, 2022 · 19 comments · Fixed by #333
Closed

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

pmkevans opened this issue Sep 8, 2022 · 19 comments · Fixed by #333
Labels
Feature New feature

Comments

@pmkevans
Copy link

pmkevans commented Sep 8, 2022

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.
@pmkevans pmkevans added the Feature New feature label Sep 8, 2022
@mikepizzo
Copy link
Member

mikepizzo commented Sep 13, 2022 •
edited
Loading

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
Copy link

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.CreateQueryglobal::Microsoft.Dynamics.DataEntities.ProcessTemplateTask(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
Copy link

magiva commented Nov 11, 2022

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
Copy link

sanderson196 commented Nov 11, 2022 via email

@magiva
Copy link

magiva commented Nov 11, 2022

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
Copy link

sanderson196 commented Nov 11, 2022 via email

@magiva
Copy link

magiva commented Nov 11, 2022

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

@parydnar parydnar mentioned this issue Nov 21, 2022
Open
@SomeTroglodyte
Copy link

SomeTroglodyte commented Dec 16, 2022 •
edited
Loading

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
Copy link

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
Copy link

magiva commented Jan 11, 2023

thanks for the update, will test soon

@gregwinterstein gregwinterstein mentioned this issue Feb 2, 2023
Merged
@dazinator
Copy link

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
Copy link

@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
Copy link

@AndreasHassing thank you much obliged!

@SomeTroglodyte
Copy link

Still broken in public VS extension 1.0.1

@AndreasHassing
Copy link

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
Copy link

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" 😸

@AndreasHassing
Copy link

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
Copy link

#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.

@gathogojr gathogojr linked a pull request Sep 25, 2023 that will close this issue
Create valid property identifiers when generating OData client #333
Merged
@adamsilenko
Copy link

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."
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Feature New feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Create valid property identifiers when generating OData client gregwinterstein/ODataConnectedService
8 participants
@AndreasHassing @dazinator @magiva @mikepizzo @pmkevans @SomeTroglodyte @sanderson196 @adamsilenko