Skip to content

[Tables] Handle boolean when using disableTypeConversion and fix value type #17385

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
Merged
joheredi merged 5 commits into from
Sep 7, 2021
Merged

[Tables] Handle boolean when using disableTypeConversion and fix value type #17385

joheredi merged 5 commits into from
Sep 7, 2021

Conversation

@joheredi
Copy link
Member

@joheredi joheredi commented Aug 31, 2021

Problem:

  1. Table service never sends type metadata for Boolean entity properties and client code was not handling it when disableTypeConversion was set to true.
  2. For types which the service returns no type metadata, when using disableTypeConversion we are returning the actual type instead of a string as expected when using disableTypeConversion . For example
// current
{value: 1, type: "Int32"};

// expected
{value: "1", type: "Int32"}

Fix:

  • Explicitly handle boolean types
  • Convert number and booleans to strings when setting the value in the entity property object

@ghost ghost added the Tables label Aug 31, 2021
ellismg
ellismg suggested changes Aug 31, 2021
View reviewed changes
Copy link
Member

@ellismg ellismg left a comment

Choose a reason for hiding this comment

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

Question about adding some additional coverage in the recorded tests to so we capture the actual service behavior. Let me know if I'm misunderstanding something.

sdk/tables/data-tables/test/public/tableclient.spec.ts Outdated
Copy link
Member

@ellismg ellismg Aug 31, 2021

Choose a reason for hiding this comment

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

Should we add a boolean to this object so we have a recorded test case with a boolean property using disableTypeConversion: true? From the description of the PR it seems like if we had that initially we would have discovered at least part of the bug?

Copy link
Member Author

@joheredi joheredi Aug 31, 2021

Choose a reason for hiding this comment

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

Yeah that's a great point, I'll update the recordings

Copy link
Member Author

@joheredi joheredi Aug 31, 2021

Choose a reason for hiding this comment

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

@ellismg updated the test and recordings to include a boolean property in the entity. Thanks for bringing this up 😄

ellismg
ellismg approved these changes Aug 31, 2021
View reviewed changes
Copy link
Member

@ellismg ellismg left a comment

Choose a reason for hiding this comment

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

New test looks good to me. I'm happy once CI is happy. Thanks for the fix here Jose!

xirzec
xirzec approved these changes Aug 31, 2021
View reviewed changes
Copy link
Member

@xirzec xirzec left a comment

Choose a reason for hiding this comment

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

Small comments but thanks for the quick fix

sdk/tables/data-tables/src/serialization.ts Outdated
Copy link
Member

@xirzec xirzec Aug 31, 2021

Choose a reason for hiding this comment

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

technically this will lose precision for some cases. Does the server return it to us as a JSON number instead of as a string so there's not much we can do?

Copy link
Member Author

@joheredi joheredi Aug 31, 2021

Choose a reason for hiding this comment

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

Yeah, the service gives us a JSON number :(

@joheredi
Copy link
Member Author

joheredi commented Sep 7, 2021

/azp run js - data-tables - test

@azure-pipelines
Copy link

azure-pipelines bot commented Sep 7, 2021

No pipelines are associated with this pull request.

@joheredi
Copy link
Member Author

joheredi commented Sep 7, 2021

/azp run js - data-tables - tests

@azure-pipelines
Copy link

azure-pipelines bot commented Sep 7, 2021

Azure Pipelines successfully started running 1 pipeline(s).

@joheredi joheredi merged commit 18baa69 into Azure:main Sep 7, 2021
openapi-sdkautomation bot pushed a commit to AzureSDKAutomation/azure-sdk-for-js that referenced this pull request Feb 16, 2022
@xwang971 @Juliehzl
@vturecek @vinisoto @erich-wang @vusause @sanchitmehta
CodeGen from PR 17820 in Azure/azure-rest-api-specs
17abd1e 
Microsoft.App version 2022-01-01-preview (Azure#17820)

* New Swagger Spec File

* New Swagger Example Spec File

* New Readme Config File

* New Azure AZ Readme Config File

* New Azure CLI Readme Config File

* New Go Language Readme Config File

* New Python Language Readme Config File

* New Typescript Language Readme Config File

* New C# Language Readme Config File

* Adding new API version 2022-01-01-preview for the new service Microsoft.App (Azure#17135)

* Adding swagger and examples

* Fix samples

* Fix linting errors

* fix errors

* fix more errors

* prettier fixes

* Fix the VNET properties

* fix the vnet props

* Attempt to remove x-ms-identifiers

* Add x-ms-identifiers back

* Revert "Add x-ms-identifiers back"

This reverts commit 44525ab5ace45d9cb1bc85bee2751f015dcaffc6.

* Addsourcecontrolapis (Azure#17287)

* add sourcecontrol apis

* remove space

* prettier fix

* typo

* avocado fix

* lint fix

* add replicas apis (Azure#17501)

* Remove Dapr components from ContainerApp spec. Not breaking because the version hasn't been released yet. (Azure#17479)

* Remove dapr components from the ContainerApp object

* Fix example

* add descriptions

* fix

* change the auto-rest parameters

* Support volume mounts for containerApp (Azure#17530)

* add volume mounts

* add identifier

* refine volume definition

* Fix samples (Azure#17534)

* Adding managed identity (Azure#17569)

* Adding managed identity

* prettier fix.

* Microsof.app 2022 01 01 preview/add custom domains (Azure#17385)

* add support for Custom domains and certificates

* add Certificates

* Ccertificate as child resource of Managed Env.

* Support default custom domain

* PUT/DELETE certificate are not long-running

* Add Custom Domain Verification Id

* domains for all revisions and adding examples

* missing examples

* one more missing example

* Examples+missing paths

* Adding missing envelope properties

* Addressing PR comments

* Removing AKV and Free cert related properties

* Prettier and semantic validation fixes

* Fixing semantic validations and examples

* More fixes

* Addressing more PR comments

* Updating examples

* fixing type

* fixing types

* Extra properties and responses

* misplaced response

* whitespace

* fix security section

* fixing ManageEnvironment securityDefinitions

* add 204 delete response

* Removing virtual IP and IP Based option

* change modelAsString

* Addressing ARM PR comments

* Removing 404 response from example

* renaming custom hostname analysis operation

* mark certificate as tracked resource

* fix sample

* Use Certificate Id instead of Certificate name

Co-authored-by: Ruslan Yakushev 🚴 <[email protected]>
Co-authored-by: vinisoto <[email protected]>

* Add new properties for ContainerApp (Azure#17483)

* add ephemeral storage

* add outbound ip

* add listsecrets

* fix CI

* fix example

* fix

* add identifier

* fix

* add example

* mars as secret

* Add storages operation for managedEnvironment (Azure#17545)

* add storage

* fix

* fix typo

* Add EasyAuth configuration APIs for ContainerApp (Azure#17492)

* Add Easy Auth Config related APIs for ContainerApp

* Use common type ProxyResource

* Update description

* update per validation

* typo fix

* fix validation error

* Update sample and description

* Update because ARM prefer string than boolean

* Add static web identity provider

* Add container probes (Azure#17535)

* Add container probes

* minor fix

* Use execute instead of exec'd, add identifier

* remove exec from preview

* use integer instead of intorstring

* Add `internal` property under VnetConfiguration for internalOnly environments (Azure#17656)

* Add internal property under VnetConfiguration for internalOnly environments

* Update examples

* Add Dapr Components collection APIs (Azure#17552)

* Add daprComponents

* update readme

* Fix linting errors

* More lint fixes

* prettier fixes

* make dapr component a tracked resource

* fix the patch

* fix lint errors

* Revert "fix lint errors"

This reverts commit 045f1d94bddf3527eab98b7a376070ab30fdd760.

* Revert "fix the patch"

This reverts commit 14521103e848e09762185f832c0270c16ed16efd.

* Revert "make dapr component a tracked resource"

This reverts commit 239268eda070ff37f26e8a772adacdd26bbf0937.

* Fix linter issues

* fix wrong fix

* fix linter

* fix the operationids (Azure#17809)

* correct resource name (Azure#17846)

* Add custom open id providers support (Azure#17855)

* Add custom open id providers support

* Update description

Co-authored-by: Xingjian Wang <[email protected]>
Co-authored-by: Zunli Hu <[email protected]>
Co-authored-by: Vaclav Turecek <[email protected]>
Co-authored-by: Vini Soto <[email protected]>
Co-authored-by: vinisoto <[email protected]>
Co-authored-by: erich-wang <[email protected]>
Co-authored-by: Mike Vu <[email protected]>
Co-authored-by: Sanchit Mehta <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xirzec xirzec xirzec approved these changes

@deyaaeldeen deyaaeldeen Awaiting requested review from deyaaeldeen

+1 more reviewer

@ellismg ellismg ellismg approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tables

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@joheredi @xirzec @ellismg