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.

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

Support reading/writing Edmx with Version=4.01 #1476

Merged
merged 1 commit into from
Jun 4, 2019

Conversation

mikepizzo
Copy link
Member

@mikepizzo mikepizzo commented Jun 4, 2019

Issues

This pull request fixes issue #1470.

Description

Support reading and writing Version=4.01 in Edmx

Checklist (Uncheck if it is not completed)

  • Test cases added
  • Build and test with one-click build and test script passed

/// <summary>
/// The default version of EDM to use if none is specified.
/// </summary>
public static Version EdmVersionDefault = EdmVersion4;
Copy link
Member

@xuzhg xuzhg Jun 4, 2019

Choose a reason for hiding this comment

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

EdmVersion4 [](start = 50, length = 11)

Shall we make the 401 as default? #ByDesign

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't want to make 4.01 the default because it would be a potentially breaking change for libraries that don't currently explicitly set the version.


In reply to: 290412054 [](ancestors = 290412054)

// Read in the CSDL and verify the version
CsdlReader.TryParse(xmlReader, out edmModel, out edmErrors);
Assert.Equal(edmErrors.Count(), 0);
Assert.Equal(edmModel.GetEdmVersion(), odataVersion == "4.0" ? EdmConstants.EdmVersion4 : EdmConstants.EdmVersion401);
Copy link
Member

Choose a reason for hiding this comment

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

EdmConstants.EdmVersion4 [](start = 75, length = 24)

Can add the Enum into the [InlineData("4.0", EdmConstants.EdmVersion4)]

Copy link
Member Author

Choose a reason for hiding this comment

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

I got an error from the test framework when I tried to add the static value to the InlineData.


In reply to: 290413640 [](ancestors = 290413640)


// Specify the model
EdmModel edmModel = new EdmModel(false);
edmModel.SetEdmVersion(odataVersion == "4.0" ? EdmConstants.EdmVersion4 : EdmConstants.EdmVersion401);
Copy link
Member

Choose a reason for hiding this comment

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

EdmConstants.EdmVersion4 [](start = 59, length = 24)

same as, can move it to [InlineData('")]

Copy link
Member Author

Choose a reason for hiding this comment

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

as above, I like that a lot better, but I got an error from the test framework when I tried to put EdmConstants.EdmVersion4 in the inline data.


In reply to: 290414340 [](ancestors = 290414340)

Copy link
Member

@xuzhg xuzhg Jun 4, 2019

Choose a reason for hiding this comment

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

Straight. It should work for Enum (It's not an enum? ). Same as:

[Theory]
[InlineData(EdmTypeKind.Complex)]
[InlineData(EdmTypeKind.Primitive)]
public void OperationReturnTypeWithCollectionOfAbstractTypeShouldError(EdmTypeKind typeKind)

[InlineData("4.01")]
public void ValidateEdmxVersions(string odataVersion)
{
string xml = "<?xml version=\"1.0\" encoding=\"utf-16\"?><edmx:Edmx Version=\"" + odataVersion + "\" xmlns:edmx=\"http://docs.oasis-open.org/odata/ns/edmx\"><edmx:DataServices /></edmx:Edmx>";
Copy link
Member

Choose a reason for hiding this comment

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

edmx:Edmx Version="" + odataVersion [](start = 70, length = 36)

does it mean "Edmx version is same as Edm version"?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea why the current library differentiates between EdmVersion and EdmxVersion; they should always be identical. Since these are public I didn't change the redundancy, but in 8.x we should eliminate the redundancy and have a cleaner model for a single version.


In reply to: 290417063 [](ancestors = 290417063)

@xuzhg xuzhg merged commit c0997c2 into OData:master Jun 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants