Skip to content

fix(api): serialize TemporalDateTime in identifier for ModelQueries.get - #7237

Closed
VarshithaPamisetty wants to merge 2 commits into
aws-amplify:mainfrom
VarshithaPamisetty:api/bug-6298-temporal-datetime-identifier
Closed

fix(api): serialize TemporalDateTime in identifier for ModelQueries.get#7237
VarshithaPamisetty wants to merge 2 commits into
aws-amplify:mainfrom
VarshithaPamisetty:api/bug-6298-temporal-datetime-identifier

Conversation

@VarshithaPamisetty

Copy link
Copy Markdown
Contributor

ModelQueries.get() throws JsonUnsupportedObjectError when an identifier has a
TemporalDateTime in it. The get path doesn't run identifier values through
_getSerializedValue like the list/filter path does, so the date never gets
stringified before JSON-encoding.

Re: #6298.

  • Serialize get variables via buildVariablesForGetRequest.
  • Serialize the composite-key get document values too (date comes out quoted).
  • Add CpkTemporalPrimaryKey test model + regression test.
  • Add matching CpkTemporalPrimaryKey model to the api-multi-auth gen2 backend.

dart analyze clean, tests red to green. Draft: live AppSync e2e still pending backend deploy + codegen.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

The get-by-ID path passed identifier values straight through without running them through _getSerializedValue (which the list/filter path uses), so a TemporalDateTime in a custom identifier was never stringified before JSON-encoding, throwing JsonUnsupportedObjectError. Serialize identifier values for both the get variables and the composite-key document. Adds a CpkTemporalPrimaryKey test model + regression test, and the matching model in the api-multi-auth gen2 backend.
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 40.97%. Comparing base (7c5ffa8) to head (4b3eb1f).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #7237   +/-   ##
=======================================
  Coverage   40.97%   40.97%           
=======================================
  Files         121      121           
  Lines        8273     8273           
  Branches     3598     3598           
=======================================
  Hits         3390     3390           
  Misses       4883     4883           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

// Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
// SPDX-License-Identifier: Apache-2.0

// ignore_for_file: public_member_api_docs, annotate_overrides, dead_code, dead_codepublic_member_api_docs, depend_on_referenced_packages, file_names, library_private_types_in_public_api, no_leading_underscores_for_library_prefixes, no_leading_underscores_for_local_identifiers, non_constant_identifier_names, null_check_on_nullable_type_parameter, override_on_non_overriding_member, prefer_adjacent_string_concatenation, prefer_const_constructors, prefer_if_null_operators, prefer_interpolation_to_compose_strings, slash_for_doc_comments, sort_child_properties_last, unnecessary_const, unnecessary_constructor_name, unnecessary_late, unnecessary_new, unnecessary_null_aware_assignments, unnecessary_nullable_for_final_variable_declarations, unnecessary_string_interpolations, use_build_context_synchronously

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just ignoring all of this is not ideal. This should be avoided.

@VarshithaPamisetty

Copy link
Copy Markdown
Contributor Author

Superseded by #7255, which is opened from an upstream branch so CI runs with the required secrets. Closing this fork PR in favor of #7255.

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.

3 participants