Skip to content

Type-safe parsing of JSON response #1127

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jclou
Copy link
Contributor

@jclou jclou commented Apr 1, 2025

Changes:

  • Migrated most PBMORTB response model objects to Swift. The remaining model objects use objc generics which are not compatible with Swift and will need further consideration to migrate. Helper functions in JSONParsing.swift keep the model object code simple and ensure consistent and type-safe conversions to and from JSON.

Additions:

  • Added API to let publishers provide their own custom model objects and response JSON handling.

Fixes:
-Made PBMORTBAbstractResponse.ext nullable to handle the case in -[PBMORTBAbstractResponse initWithJsonDictionary:extParser] where it does not get set. This case previously existed in tests, but after migrating PBMORTBBidExt to swift, the nil objc class instance instead became an uninitialized Swift class instance and crashed.

  • Added nil check to Swift function call in -[PBMAbstractCreative handleClickthrough:sdkConfiguration:completionHandler:onExit:]

Copy link
Contributor

@YuriyVelichkoPI YuriyVelichkoPI left a comment

Choose a reason for hiding this comment

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

Please, remove PBM prefix in new Swift classes.

Since we are switching converting JSON to Swift. can we use advantages of Codable?

@jclou
Copy link
Contributor Author

jclou commented Apr 28, 2025

Its possible to use codable, but in my experience Codable tends to be too strict for most uses. In our case I think we want to prioritize successful parsing of response objects over enforcing strict conformance to the response spec. If for example for some field the response contains "true" instead of the expected true, we would probably want to fallback to a default value rather of failing to decode the entire object.

You could loosen strictness in these cases, but this is not the default behavior and you would need to write your own custom decoding logic and the end result is usually harder to read and more error prone than alternatives.

@jclou
Copy link
Contributor Author

jclou commented Apr 28, 2025

@YuriyVelichkoPI Can we hold up on this PR and prioritize merging in the SPM migration PRs? Those PRs include the swift migration part of this PR, and I'd like to avoid having to do a rebase/merge. I can add back in the custom class feature afterwards.

@YuriyVelichkoPI
Copy link
Contributor

Its possible to use codable, but in my experience Codable tends to be too strict for most uses. In our case I think we want to prioritize successful parsing of response objects over enforcing strict conformance to the response spec. If for example for some field the response contains "true" instead of the expected true, we would probably want to fallback to a default value rather of failing to decode the entire object.

You could loosen strictness in these cases, but this is not the default behavior and you would need to write your own custom decoding logic and the end result is usually harder to read and more error prone than alternatives.

Thanks for the clarification. Makes sense. We can try Codable later.

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.

2 participants