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

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

Merged
merged 3 commits into from
Apr 27, 2020
Merged

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

merged 3 commits into from
Apr 27, 2020

Conversation

paulodero
Copy link
Contributor

@paulodero paulodero commented Feb 5, 2020 •
edited
Loading

…t header to an OData service.

Issues

*This pull request fixes issue #725 and #522 *

Description

Enable OData client to send IEEE754Compatible parameter in the request header. This is to ensure that both the service and the client are in sync on how long and decimal values in a collection are serialized. In the header, this value can be set to true or false. If set to true then long and decimals will be serialized as strings.

Checklist (Uncheck if it is not completed)

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

Additional work necessary

If documentation update is needed, please add "Docs Needed" label to the issue and provide details about the required document change in the issue.

@paulodero paulodero marked this pull request as ready for review February 5, 2020 16:53
@paulodero paulodero added the Ready for review Use this label if a pull request is ready to be reviewed label Feb 5, 2020
xuzhg
xuzhg requested changes Feb 5, 2020
View reviewed changes
Copy link
Member

@xuzhg xuzhg left a comment

Choose a reason for hiding this comment

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

🕐

@paulodero paulodero self-assigned this Feb 6, 2020
@paulodero paulodero requested a review from xuzhg February 7, 2020 08:58
xuzhg
xuzhg reviewed Feb 19, 2020
View reviewed changes
src/Microsoft.OData.Core/Uri/ODataUriUtils.cs
/// <param name="isIeee754Compatible">true if value should be IEEE 754 compatible.</param>
/// <returns>A string representation of <paramref name="value"/> for use in a Url.</returns>
[SuppressMessage("Microsoft.Design", "CA1055:UriReturnValuesShouldNotBeStrings", Justification = "designed to aid the creation on a URI, not create a full one")]
public static string ConvertToUriLiteral(object value, ODataVersion version, bool isIeee754Compatible)
Copy link
Member

Choose a reason for hiding this comment

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

bool isIeee754Compatible [](start = 85, length = 24)

Can we consider to create a "ConvertSetting" class and add "IsIeee754Compatible" as one of this setting.

The advantage is that, in the future if we have more configuration, we don't need to change the syntax of the method, we can just modify the ConvertSetting ?

Copy link

Choose a reason for hiding this comment

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

I was thinking an enum of possible conversion options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@odero an enum would restrict the type of settings. Remember a good design requires an enum to hold related values like days in a week. In this case we are bound to have a number of varied types/values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xuzhg Do we see an increase in these configurations in future? At the moment we have very few configs for the function.

Copy link
Member

Choose a reason for hiding this comment

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

So far no, :)

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@paulodero I think a settings object would also be a better idea even if we only have one option. The reason I think so is because IsIeee754Compatible accounts for special case, applies only for numbers. In that sense, it doesn't seem like a "main" option and seem more suited to be part of a larger settings object than standalone parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will be good to create an object for it if we have more settings.

odero
odero requested changes Feb 21, 2020
View reviewed changes
Copy link

@odero odero left a comment •
edited
Loading

Choose a reason for hiding this comment

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

  1. You've linked to Microsoft.OData.Client request failure when calling a function requiring array parameter #725 which doesn’t seem to be the issue this PR fixes
  2. What about handling Accept headers? You only seem to check content-type

"If the IEEE754Compatible is specified in the Accept header, we should always honor that." - mikepizzo (OData/WebApi#53 (comment)) Maybe 'Accept' is already supported. Would be good to confirm

@paulodero
Copy link
Contributor Author

You've linked to #725 which doesn’t seem to be the issue this PR fixes
What about handling Accept headers? You only seem to check content-type

"If the IEEE754Compatible is specified in the Accept header, we should always honor that." - mikepizzo (OData/WebApi#53 (comment)) Maybe 'Accept' is already supported. Would be good to confirm

@odero That is the correct issue this PR is fixing.

@paulodero paulodero requested review from odero, xuzhg and g2mula February 25, 2020 06:19
g2mula
g2mula reviewed Feb 25, 2020
View reviewed changes
Copy link
Member

@g2mula g2mula left a comment

Choose a reason for hiding this comment

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

Generally looks good to me, ping me to approve once the highlighted issues are resolved.

@paulodero paulodero requested a review from g2mula February 26, 2020 11:08
g2mula
g2mula approved these changes Feb 26, 2020
View reviewed changes
Copy link
Member

@g2mula g2mula left a comment

Choose a reason for hiding this comment

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

LGTM

xuzhg
xuzhg reviewed Feb 26, 2020
View reviewed changes
src/Microsoft.OData.Client/DataServiceContext.cs
/// <summary>
/// Gets IsIeee754Compatible header value.
/// </summary>
internal bool IsIeee754Compatible { get; private set; }
Copy link
Member

Choose a reason for hiding this comment

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

I am wondering why do we need to save "Ieee...Compatible" in a ServiceContext?

A service context is a context for all potential request. The state should not change. Now, you have to change this member every time for every request. I don't think it's a good place for it.
@michael Pizzo

Copy link
Contributor Author

@paulodero paulodero Apr 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@mikepizzo Do you have any reservations on this? The tricky part is how we get to pass this parameter around to the Serializer.cs if we do not set it in DataServiceContext.

xuzhg
xuzhg reviewed Feb 26, 2020
View reviewed changes
src/Microsoft.OData.Client/Serialization/Serializer.cs Outdated
@@ -750,7 +750,9 @@ private string ConvertToEscapedUriValue(string paramName, object value, bool use
// to determine which IEdmOperationImport to pass to the ODL writer. So the ODL writer is
// serializing the parameter payload without metadata. Setting the model to null so ODL doesn't
// do unnecessary validations when writing without metadata.
string literal = ODataUriUtils.ConvertToUriLiteral(valueInODataFormat, CommonUtil.ConvertToODataVersion(this.requestInfo.MaxProtocolVersionAsVersion), null /* edmModel */);
bool isIeee754Compatible = this.requestInfo.Context.IsIeee754Compatible;
Copy link
Member

Choose a reason for hiding this comment

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

requestInfo [](start = 44, length = 11)

I'd like to add "IsIeee754Com..." into "RequestInfo"?

xuzhg
xuzhg reviewed Feb 26, 2020
View reviewed changes
src/Microsoft.OData.Core/ODataMessageWriterSettings.cs

/// If this is set to true then long and decimals will be serialized as strings.
/// </summary>
internal bool IsIeee754Compatible { get; set; }
Copy link
Member

@xuzhg xuzhg Feb 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

internal bool IsIeee754Compatible { get; set; } [](start = 8, length = 47)

add this into "CopyFrom" method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@paulodero paulodero force-pushed the master-IEEE754Compatible branch from ba6b5c7 to 27b8167 Compare March 9, 2020 08:22
@marabooy marabooy requested a review from mikepizzo March 10, 2020 16:51
@marabooy marabooy added this to the 7.7.0 milestone Mar 10, 2020
@paulodero paulodero force-pushed the master-IEEE754Compatible branch 2 times, most recently from b3177a7 to eb24759 Compare April 23, 2020 20:31
@paulodero paulodero force-pushed the master-IEEE754Compatible branch from eb24759 to ac96433 Compare April 24, 2020 07:17
@xuzhg
Copy link
Member

xuzhg commented Apr 24, 2020

rebase, squash and merge, thanks.

@paulodero paulodero merged commit e0e628a into OData:master Apr 27, 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 <[email protected]>
Date:   Wed May 6 21:39:03 2020 -0700

    Fix E2E Tests

commit 565ccdc
Author: Sreejith Pazhampilly <[email protected]>
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 <[email protected]>
Date:   Tue Apr 28 16:33:36 2020 -0700

    Make the CI working

commit d844d4a
Author: Sam Xu <[email protected]>
Date:   Thu Apr 23 22:09:17 2020 -0700

    Update the test case projects

commit 26709dd
Author: Sreejith Pazhampilly <[email protected]>
Date:   Thu Apr 23 12:09:39 2020 -0700

    updates , pipeline changes

commit 21a1d3d
Author: Sam Xu <[email protected]>
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 <[email protected]>
Date:   Wed Apr 22 15:01:40 2020 -0700

    update the build CI and project

commit c9fae43
Author: Sam Xu <[email protected]>
Date:   Tue Apr 21 15:21:30 2020 -0700

    Clean up the codes

commit 6c50be0
Author: Sam Xu <[email protected]>
Date:   Tue Apr 21 14:56:13 2020 -0700

    With whole project changes

commit 77d9f88
Author: Paul Odero <[email protected]>
Date:   Thu May 7 10:52:23 2020 +0300

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

commit 5a20f51
Author: Sam Xu <[email protected]>
Date:   Mon May 4 16:10:36 2020 -0700

    Resolve the product code build warnings

commit 26739c1
Author: Sam Xu <[email protected]>
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 <[email protected]>
Date:   Mon Apr 27 11:52:23 2020 +0300

    Fixing issue #794 (#1656)

commit e0e628a
Author: Paul Odero <[email protected]>
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 <[email protected]>
Date:   Fri Apr 17 11:48:34 2020 -0700

    Fix some build warning in Edm lib

commit 82ed887
Author: Clément Habinshuti <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>

commit 80e73d0
Author: KanishManuja-MS <[email protected]>
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

@g2mula g2mula g2mula approved these changes

@xuzhg xuzhg xuzhg approved these changes

@KanishManuja-MS KanishManuja-MS Awaiting requested review from KanishManuja-MS

@habbes habbes Awaiting requested review from habbes

@marabooy marabooy Awaiting requested review from marabooy

@ElizabethOkerio ElizabethOkerio Awaiting requested review from ElizabethOkerio

@mikepizzo mikepizzo Awaiting requested review from mikepizzo

Assignees

@paulodero paulodero

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.
6 participants
@paulodero @xuzhg @odero @g2mula @habbes @marabooy