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 non-JSON object decodable values in getMetadataValue #4555

Merged
merged 5 commits into from
Dec 6, 2024

Conversation

codykerns
Copy link
Member

@codykerns codykerns commented Dec 5, 2024

Checklist

  • If applicable, unit tests
  • If applicable, create follow-up issues for purchases-android and hybrids

Motivation

When fetching an offering metadata value with getMetadataValue(for:) (without a default value), a primitive Decodable (like String) causes the JSON decoder to crash, as it assumes the value is a JSON object, but it isn't a valid top-level JSON type.

Description

  • Use JSONSerializer's isValidJSONObject method before attempting to serialize values
  • If it's not a valid JSON object, attempt to return the metadata value as the provided decodable type

@codykerns codykerns added the pr:fix A bug fix label Dec 5, 2024
Copy link
Member

@aboedo aboedo left a comment

Choose a reason for hiding this comment

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

looks great so far! We should add a test to ensure we don't break it again in the future and then ready to merge

Sources/Purchasing/Offering.swift Outdated Show resolved Hide resolved
@codykerns codykerns marked this pull request as ready for review December 6, 2024 15:55
@codykerns codykerns requested a review from aboedo December 6, 2024 15:55
Copy link
Member

@aboedo aboedo left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this!!

@codykerns codykerns merged commit 7c34072 into main Dec 6, 2024
10 checks passed
@codykerns codykerns deleted the cody/support-primitive-metadata-values branch December 6, 2024 22:00
@nyeu
Copy link
Contributor

nyeu commented Dec 9, 2024

Hey @aboedo is there a reason why getMetadataValue<T>(for key: String, default: T) -> T is not set as Decodable? I was about to open a PR with this change but maybe there's a reason I'm missing here.

Here's what I think it should be #4561

I don't know why these 2 methods should be different while one is only providing a default value. Otherwise I feel the name should explicit the difference. Wdyt?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:fix A bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants