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

Defining a CustomScalarType which is [String:Any] #3369

Closed
solidcell opened this issue Apr 16, 2024 · 12 comments
Closed

Defining a CustomScalarType which is [String:Any] #3369

solidcell opened this issue Apr 16, 2024 · 12 comments
Assignees
Labels
needs investigation question Issues that have a question which should be addressed

Comments

@solidcell
Copy link

Question

I'm migrating from 0.51.2 to 1.10.0 and I'm having a tough time converting a custom scalar to the new system.

We used to have a simple public typealias JSON = [String: Any] which was all we needed. However, now that doesn't suffice since we get errors that JSON needs to conform to SelectionSetEntityValue and OutputTypeConvertible.

This migration is quite the beast, so even if I get this JSON scalar related code to compile, I'm not so confident with how it should behave with errors and casting and such. Any help would be very much appreciated.

@solidcell solidcell added the question Issues that have a question which should be addressed label Apr 16, 2024
@calvincestari
Copy link
Member

Hi @solidcell - can you share the code you've got so far for your custom scalar?

I agree moving to 1.0 is a large migration; have you read the 1.0 migration guide that lays out everything that you may need to change during the migration?

@solidcell
Copy link
Author

solidcell commented Apr 19, 2024

Yea I've already read the migration guide.

The easiest way I've found to get a first pass which compiles, and sort of works is to not use a typealias directly on [String:Any], but to make it a wrapper struct. However that has its disadvantages. Here's what I've got so far:

public struct JSON: CustomScalarType, SelectionSetEntityValue {

    public init(_fieldData: AnyHashable?) {
        guard let _fieldData else {
            fatalError()
        }
        self.value = _fieldData
    }

    public init(_jsonValue value: JSONValue) throws {
        self.value = value
    }
    
    public init(anyHashable: JSONValue) {
        self.value = anyHashable
    }

    public var _fieldData: AnyHashable {
        value
    }

    public var _jsonValue: JSONValue {
        value
    }

    public var _asAnyHashable: AnyHashable {
        value
    }

    public subscript<T>(_ key: String) -> T? {
        guard let dict = value as? [String : Any],
              let casted = dict[key] as? T
        else { fatalError() }
        return casted
    }

    public func compactMap<T>(
        _ transform: ((key: String, value: Any)) throws -> T?
    ) rethrows -> [T] {
        guard let dict = value as? [String : Any]
        else { fatalError() }
        return try dict.compactMap(transform)
    }

    public func compactMapValues<T>(
        _ transform: (Any) throws -> T?
    ) rethrows -> [String : T] {
        guard let dict = value as? [String : Any]
        else { fatalError() }
        return try dict.compactMapValues(transform)
    }

    public var values: Dictionary<String, Any>.Values {
        guard let dict = value as? [String : Any]
        else { fatalError() }
        return dict.values
    }

    public var description: String {
        guard let dict = value as? [String : Any]
        else { fatalError() }
        return dict.description
    }

    private let value: JSONValue
}

I haven't refactored this up yet since I don't like the direction this solution is going. It's enabled me to get the app compiling and running again, but there are a few issues I've noticed so far:

  1. Needing to redefine and delegate methods (compactMap, subscript, etc.) to the wrapped Dictionary is getting out of hand and isn't elegant.
  2. It's a bit awkward that a JSON field from a result is treated like JSON wrapper type, but if it contains nested JSON itself, that nested JSON accessed with subscript won't technically be JSON (the wrapper type), but just a regular AnyHashable([String : Any]). Not the end of the world, but it's awkward that with this solution the types for top-level and nested JSON hashes are different, so you need to be careful about casting and what to expect. And I don't think wrapping it with the type explicitly upon returning in subscript is great either.

I would much prefer to be able to simply work with the underlying [String : Any] type as I had been doing with Apollo 0.x. I'm hoping I can get back to that. However, trying to do that is giving me a whole host of problems. So I'm not sure if it's the better solution in the end or if it's even possible.

Doing this, for instance:

public typealias JSON = [String : Any]

extension JSON: CustomScalarType {}

Is giving me:

'CustomScalarType' requires the types 'Any' and 'any GraphQLOperationVariableValue' be equivalent
Conditional conformance of type 'Dictionary<Key, Value>' to protocol 'CustomScalarType' does not imply conformance to inherited protocol 'AnyScalarType'
Conditional conformance of type 'Dictionary<Key, Value>' to protocol 'CustomScalarType' does not imply conformance to inherited protocol 'OutputTypeConvertible'
Did you mean to explicitly state the conformance with different bounds?
Type 'Dictionary<Key, Value>' does not conform to protocol 'CustomScalarType'

I'm just starting to dive deeper, but something tells me I'm fighting against the current. The types and protocols in Apollo 1.x are a lot to take in, so I'm just trying to wrap my head around it. For instance, the first error: 'CustomScalarType' requires the types 'Any' and 'any GraphQLOperationVariableValue' be equivalent. The compiler isn't being particularly helpful and it's not clear to me how CustomScalarType is making that requirement in the first place. Maybe some extension being defined somewhere on Dictionary which is influencing this conformance.. It's hard to say.

@calvincestari
Copy link
Member

  1. Needing to redefine and delegate methods (compactMap, subscript, etc.) to the wrapped Dictionary is getting out of hand and isn't elegant.

I'm not sure why you need to be defining conformance to SelectionSetEntityValue, that might be compounding the issues. Any as the element is not helping though, it's too broad. What about using AnyHashable instead?

  struct JSON: CustomScalarType, Hashable {
    private let wrapped: [String: AnyHashable]

    init(_jsonValue value: ApolloAPI.JSONValue) throws {
      guard let value = value as? [String: AnyHashable] else { throw JSONDecodingError.wrongType }

      self.wrapped = value
    }
    
    var _jsonValue: ApolloAPI.JSONValue { wrapped }
  }
  1. It's a bit awkward that a JSON field from a result is treated like JSON wrapper type, but if it contains nested JSON itself, that nested JSON accessed with subscript won't technically be JSON (the wrapper type), but just a regular AnyHashable([String : Any]). Not the end of the world, but it's awkward that with this solution the types for top-level and nested JSON hashes are different, so you need to be careful about casting and what to expect. And I don't think wrapping it with the type explicitly upon returning in subscript is great either.

Can you show me how you ideally want to be able to access the custom scalar value? I'm not sure there is any way around having to cast the type out of the subscript when using the value type of Any, and you will always need to be careful because there is no type safety in Any.

Custom scalars are for your code to define and the type safety you get from that is entirely up to what your custom type provides. If you want that type to be generated for you then it needs to be fully defined in the schema so codegen can work with it.

@solidcell
Copy link
Author

If I don't conform to SelectionSetEntityValue, I get this error:
image

Using AnyHashable instead of Any as the value didn't change anything (either better or worse), so I'll leave it as AnyHashable. That was my plan, but I was trying to be as incremental in my migration as possible. So I was leaving it as Any for now until I got everything working again, since that's what it was pre-migration.

I'm not sure there is any way around having to cast the type out of the subscript when using the value type of Any

Ah, misunderstanding here. It's not my intention to get more type safety from Any than I should expect. I was just now trying to type up exactly what I was after, but I was on a wrong path. So nevermind my #2.

In the end, it would just be nice to just have #1. A typealias would allow me to stop having this need to delegate everything and just simply have the type I'm really after. Something like what's possible for Date:

public typealias Date = Foundation.Date

extension Foundation.Date: CustomScalarType {
    public init (_jsonValue value: JSONValue) throws {
        guard let string = value as? String else {
            throw JSONDecodingError.couldNotConvert(value: value, to: String.self)
        }

        guard let date = Date(jsonString: string) else {
            throw JSONDecodingError.couldNotConvert(value: string, to: Date.self)
        }

        self = date
    }

...

But using a typealias gives me:

'CustomScalarType' requires the types 'AnyHashable' and 'any GraphQLOperationVariableValue' be equivalent
Conditional conformance of type 'Dictionary<Key, Value>' to protocol 'CustomScalarType' does not imply conformance to inherited protocol 'AnyScalarType'
Conditional conformance of type 'Dictionary<Key, Value>' to protocol 'CustomScalarType' does not imply conformance to inherited protocol 'OutputTypeConvertible'
Did you mean to explicitly state the conformance with different bounds?
Type 'Dictionary<Key, Value>' does not conform to protocol 'CustomScalarType'

@calvincestari
Copy link
Member

@solidcell - have you managed to make any progress on this or is it still an issue? I'm wondering if the other issue we've worked on been able to help you with this one too?

@AnthonyMDev
Copy link
Contributor

What if you change your scalar type to [String: AnyHashable]? I think that might work out of the box. We needed to ensure the values were Hashable in 1.0.

@calvincestari
Copy link
Member

@solidcell below is a sample project I put together that uses a custom scalar named JSON of type [String: AnyHashable], including a test getting data out of it to ensure it parses as expected.

CustomScalar-AnyHashable-SampleProject.zip

I'm going to close this issue with supplying the working sample project but if this doesn't suit your needs let me know in another comment on this issue and I'll pick it up again.

Copy link
Contributor

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo iOS usage and allow us to serve you better.

@TizianoCoroneo
Copy link
Contributor

  struct JSON: CustomScalarType, Hashable {
    private let wrapped: [String: AnyHashable]

    init(_jsonValue value: ApolloAPI.JSONValue) throws {
      guard let value = value as? [String: AnyHashable] else { throw JSONDecodingError.wrongType }

      self.wrapped = value
    }
    
    var _jsonValue: ApolloAPI.JSONValue { wrapped }
  }

I independently wrote almost exactly this snippet today to test Contentful's GraphQL API.

public struct JSONScalar: Hashable, CustomScalarType {
    public let values: [String: AnyHashable]

    public init(_jsonValue value: JSONValue) throws {
        guard let object = value as? [String: AnyHashable] else {
            throw JSONDecodingError.couldNotConvert(value: value, to: [String: AnyHashable].self)
        }

        self.values = object
    }

    public var _jsonValue: JSONValue {
        self.values as JSONValue
    }
}

While developing stuff around this I also encountered the same issue that @solidcell mentioned ("JSON must conform to SelectionSetEntityValue"), but I noticed that adding a conformance to Hashable fixes all issues. After a clean. And a "Reset package caches". And closing/reopening Xcode for good measure

@calvincestari
Copy link
Member

calvincestari commented May 23, 2024

While developing stuff around this I also encountered the same issue that @solidcell mentioned ("JSON must conform to SelectionSetEntityValue"), but I noticed that adding a conformance to Hashable fixes all issues.

We have this section in the Custom Scalars documentation that talks about Hashable conformance but that makes it sound optional rather than required. We may need to update that be more explicit that but I'll take a look at the code too and ensure we're not missing anything.

@solidcell
Copy link
Author

solidcell commented May 31, 2024

@calvincestari Thanks for the example.

a custom scalar named JSON of type [String: AnyHashable]

Maybe I'm misunderstanding this, but that's not quite true. It's still a wrapper around [String: AnyHashable], not [String: AnyHashable] itself. This is unlike what I've seen done for something like Date:

public typealias Date = Foundation.Date

extension Foundation.Date: CustomScalarType {
    // ....

Your code improves the implementation I had going, but unfortunately since it's still really just wrapping a [String: AnyHashable] and not [String: AnyHashable] itself, I can't subscript on a JSON directly. So for instance, this won't work:

model.everything?.first?.custom?["aKey"] (nor compactMap, keys, etc.)

So I'm still having to delegate to the underlying wrapped with a bunch of repetitive shim methods.

Also, since I'm getting back a JSON?, it's way more likely that I'll make a mistake once I start diving into it. To clarify, custom returns me a JSON?, but then once I subscript (with my shim method on JSON) to get the value at "lastKey", I now get an AnyHashable? which is really a [String:String], not a JSON? wrapping a [String:String]. So assuming I get the subscript shim in place, it's unfortunate that doing something like the following will start to be an issue only tests or manual QA will uncover:

if let nestedJSON = model.everything?.first?.custom?["lastKey"] as? JSON {
    // BUG! After updating Apollo and the implementation of JSON, we'll never actually go in here anymore, since the value for "lastKey" is not actuallly `[String: String]` anymore, but a wrapper around it.
    // So now I need to always be careful if I mean `JSON` or `[String: AnyHashable]` and I need to carefully comb my current codebase for changes to make like this.
}

An example of one of many changes I've needed to make in our codebase to account for this:

Original (Apollo 0.x):

if let a = json["key"] as? JSON { //...

Now required modification (Apollo 1.x):

if let a = json.wrapped["key"] as? [String : AnyHashable] { //...

Unfortunately I've already run into new bugs that cropped up for this reason. Unfortunately our API uses this JSON scalar at all, but that's just how it is. So hopefully I've found all the places that need to be updated, but I'm also hoping I can just change it back to just using JSON in all the places instead of being stuck in this awkward in-between two types situation.

@calvincestari
Copy link
Member

OK, I understand better what you're getting at now. Let me see what I can get working here..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs investigation question Issues that have a question which should be addressed
Projects
None yet
Development

No branches or pull requests

4 participants