Skip to content

Fix bug where open types are not identified as such during serialization - #1727

Merged
gathogojr merged 2 commits into
OData:masterfrom
gathogojr:bug/1726-open-type-serialization-of-undeclared-property
Apr 9, 2020
Merged

Fix bug where open types are not identified as such during serialization#1727
gathogojr merged 2 commits into
OData:masterfrom
gathogojr:bug/1726-open-type-serialization-of-undeclared-property

Conversation

@gathogojr

@gathogojr gathogojr commented Mar 31, 2020

Copy link
Copy Markdown
Contributor

Issues

This pull request fixes issue #1726.

Description

This PR fixes a bug in OData client library where open types IsOpen property get initialized to false specifically in a Post scenario where no query operation to the service has been done prior. This later causes an exception to be thrown during serialization since the owning type isn't recognized as open and the undeclared property won't also be interpreted as an open property.

The fix involved moving the code for populating the EdmStructuredSchemaElements property of ClientEdmModel to a position right after the service model has been loaded

Bug was inadvertently introduced in this PR and only occurs if query operation is done prior to a post operation.
Another user dissected and raised the issue here

Checklist (Uncheck if it is not completed)

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

@gathogojr gathogojr added the Ready for review Use this label if a pull request is ready to be reviewed label Mar 31, 2020
xuzhg
xuzhg reviewed Apr 1, 2020
View reviewed changes
Comment thread test/EndToEndTests/Tests/Client/Build.Desktop/ClientTests/ClientOpenTypeUpdateTests.cs
contextWrapper.Configurations.RequestPipeline.OnEntryStarting(ea => EntryStarting(ea));

contextWrapper.AddObject("Row", row);
contextWrapper.SaveChanges();

@xuzhg xuzhg Apr 1, 2020

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.

cann't use Assert.NotThrows<...>(....) pattern?

@gathogojr gathogojr Apr 1, 2020 •
edited by xuzhg
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hey @xuzhg I assume you're talking something like this. Top of my head, I don't think Microsoft.VisualStudio.QualityTools.UnitTestFramework supports Assert.NotThrows out of the box. Be that as it may, asserting that an exception is not thrown is frowned upon. They actually got rid of it in xUnit - see this thread. Any uncaught exception will fail the test hence I'd understand the argument against it. I can however improvise if you feel strongly about it. #Pending

@xuzhg xuzhg Apr 8, 2020

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.

I'd like to get rid of the current [TestMethod] and use "xunit.net" for all. Spatial, Edm, Core are moved to xunit already.
So, @odero, can you create a task to switch to xunit for the client test cases?

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

@gathogojr
gathogojr force-pushed the bug/1726-open-type-serialization-of-undeclared-property branch from 36afd13 to 1752061 Compare April 2, 2020 12:59
xuzhg
xuzhg approved these changes Apr 8, 2020
View reviewed changes

@xuzhg xuzhg left a comment

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.

:shipit:

@gathogojr
gathogojr force-pushed the bug/1726-open-type-serialization-of-undeclared-property branch from 873a27e to 06d4609 Compare April 9, 2020 06:24
@gathogojr
gathogojr merged commit d3769fd into OData:master Apr 9, 2020
@gathogojr
gathogojr deleted the bug/1726-open-type-serialization-of-undeclared-property branch April 9, 2020 07:59
@gathogojr gathogojr mentioned this pull request Apr 9, 2020
Closed
@odero odero added this to the 7.7.0 milestone Apr 22, 2020
mikepizzo added a commit that referenced this pull request May 7, 2020
@mikepizzo
Squashed commit of the following:
34d10e5 
commit 182964d
Author: Sreejith Pazhampilly 
Date:   Wed May 6 21:39:03 2020 -0700

    Fix E2E Tests

commit 565ccdc
Author: Sreejith Pazhampilly 
Date:   Thu Apr 30 11:03:23 2020 -0700

    Update to net core 2.1 and 3.1 for UT

commit afeb7d8
Author: Sreejith Pazhampilly 
Date:   Tue Apr 28 16:33:36 2020 -0700

    Make the CI working

commit d844d4a
Author: Sam Xu 
Date:   Thu Apr 23 22:09:17 2020 -0700

    Update the test case projects

commit 26709dd
Author: Sreejith Pazhampilly 
Date:   Thu Apr 23 12:09:39 2020 -0700

    updates , pipeline changes

commit 21a1d3d
Author: Sam Xu 
Date:   Wed Apr 22 18:27:12 2020 -0700

    Modify the unit test case, fix the waring, fix the version, etc

commit 82f949d
Author: Sam Xu 
Date:   Wed Apr 22 15:01:40 2020 -0700

    update the build CI and project

commit c9fae43
Author: Sam Xu 
Date:   Tue Apr 21 15:21:30 2020 -0700

    Clean up the codes

commit 6c50be0
Author: Sam Xu 
Date:   Tue Apr 21 14:56:13 2020 -0700

    With whole project changes

commit 77d9f88
Author: Paul Odero 
Date:   Thu May 7 10:52:23 2020 +0300

    Fix #1756 (#1764)- Reading OData Error Response in OData Client

commit 5a20f51
Author: Sam Xu 
Date:   Mon May 4 16:10:36 2020 -0700

    Resolve the product code build warnings

commit 26739c1
Author: Sam Xu 
Date:   Mon May 4 13:48:50 2020 -0700

    Fix WebApi issue OData/WebApi#2136: IN operator with double quote fails

commit 44ed8db
Author: Paul Odero 
Date:   Mon Apr 27 11:52:23 2020 +0300

    Fixing issue #794 (#1656)

commit e0e628a
Author: Paul Odero 
Date:   Mon Apr 27 09:36:08 2020 +0300

    Enable OData client to send IEEE754Compatible parameter in the reques… (#1659)

    * Fixes #725 and also fixes #522

commit a398670
Author: Sam Xu 
Date:   Fri Apr 17 11:48:34 2020 -0700

    Fix some build warning in Edm lib

commit 82ed887
Author: Clément Habinshuti 
Date:   Wed Apr 15 13:39:56 2020 +0300

    Add support for relative uris and absolute uris with host header in json batch requests (#1740)

commit b17d455
Author: Clément Habinshuti 
Date:   Wed Apr 15 11:40:01 2020 +0300

    Update error message when adding unsupported query option (#1729)

    * Update error message when adding unsupported query option

    * Update string resources in portablelib

    * Fix resource string errors in portable lib

    * Update error message

    * Remove references to unused error message

    * Fix failing tests

    * Fix typo

commit d3769fd
Author: John Gathogo 
Date:   Thu Apr 9 10:55:38 2020 +0300

    Fix bug where open types are not identified as such during serialization (#1727)

    * Fix bug where open types are not identified as such during serialization

    Remove unnecessary assert

    * serviceModel may be initialized from 2 locations. Ensure both trigger population of edm structured elements

    Co-authored-by: John Gathogo 

commit 80e73d0
Author: KanishManuja-MS <41647051+KanishManuja-MS@users.noreply.github.com>
Date:   Fri Apr 3 10:10:43 2020 -0700

    Revert "ODataMessageWriter can't dispose the stream if there's no write method called (#1714)"

    This reverts commit 3e02e30.

# Conflicts:
#	test/FunctionalTests/Microsoft.OData.Core.Tests/ScenarioTests/Writer/JsonLight/ODataJsonLightInheritComplexCollectionWriterTests.cs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@odero odero odero approved these changes
@marabooy marabooy marabooy approved these changes
@g2mula g2mula Awaiting requested review from g2mula
@KenitoInc KenitoInc Awaiting requested review from KenitoInc
@mikepizzo mikepizzo Awaiting requested review from mikepizzo
@ElizabethOkerio ElizabethOkerio Awaiting requested review from ElizabethOkerio
@paulodero paulodero Awaiting requested review from paulodero
@wandeg wandeg Awaiting requested review from wandeg
+1 more reviewer
@xuzhg xuzhg xuzhg approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Ready for review Use this label if a pull request is ready to be reviewed

Projects

None yet

Milestone

7.7.0

Development

Successfully merging this pull request may close these issues.

4 participants

@gathogojr @odero @marabooy @xuzhg