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 SelectionSet to receive [String: Any] for initialization when create mock object #102

Merged
merged 16 commits into from
Nov 8, 2023

Conversation

Cookiezby
Copy link
Contributor

@Cookiezby Cookiezby commented Oct 25, 2023

Relate to issue apollographql/apollo-ios#3264

If the data object which has type [String: AnyHashable] includes nested dictionary object, the compile will throw error, but it is fine to define the object with [String: Any], however, the DataDict only accept [String: AnyHashable] so there need a type conversion from [String: Any] to [String: AnyHashable]

I am not sure if it is better to add new convenience init method for DataDict type, so I only provide an extension as a help method

@@ -3,3 +3,18 @@ public extension Dictionary {
lhs.merge(rhs) { (_, new) in new }
}
}

public extension Dictionary where Key == String, Value == Any {
Copy link
Member

Choose a reason for hiding this comment

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

Thanks @Cookiezby, I'm not sure how the rest of the team feels about adding this yet but this is probably something that should live in ApolloTestSupport (here) rather than the main Apollo iOS package.

@AnthonyMDev
Copy link
Contributor

Thanks for much for the contribution @Cookiezby!

I think that this issue might be a little more complicated than this PR addresses though!

For what you are doing, you should actually be using the special json initializer we have in RootSelectionSet+JSONInitializer.swift. Because nested selections sets and custom scalars aren’t going to be properly transformed if you just initialize the DataDict with your own JSON data.

The example in our docs here should probably be updated to use that as well. However the issue #3264 won't be addressed by that alone, as it still takes in a [String: AnyHashable] dictionary.

We should probably add another method there that takes in a [String: Any] json dict and does a conversion of that internally. Rather than the proposed .asAnyHashable being public as this PR suggests.

We also should include the utility requested in this issue to convert the selection sets back into raw JSON.

If you would like to take a stab at making the suggested changes, please let me know, and feel free to update this PR.

@AnthonyMDev
Copy link
Contributor

It's worth noting that we already have AnyHashableConvertible as a protocol in ApolloAPI that could likely be used for this purpose as well (under the hood of the function implementation).

@Cookiezby
Copy link
Contributor Author

Cookiezby commented Oct 25, 2023

@calvincestari @AnthonyMDev Hi, thanks for your reviews, I am willing to update my PR for the suggested change, will check the detail about the RootSelectionSet+JSONInitializer.swift.

We should probably add another method there that takes in a [String: Any] json dict and does a conversion of that internally. Rather than the proposed .asAnyHashable being public as this PR suggests

@Cookiezby
Copy link
Contributor Author

Cookiezby commented Oct 25, 2023

@AnthonyMDev Hi, can you help to confirm if I totally understand your suggestions

For what you are doing, you should actually be using the special json initializer we have in RootSelectionSet+JSONInitializer.swift. Because nested selections sets and custom scalars aren’t going to be properly transformed if you just initialize the DataDict with your own JSON data.

Suppose I have a generated data structure

struct Document: SelectionSet {
   //.......
}

Do you mean we should init the Document by using the initialization method defined in SelectionSet+JSONInitializer.swift instead of passing a DataDict to initialize the data

// ✅ 
let jsonObject: JSONObject = [:]
let mockDocument = Document(data: jsonObject)

// ❌
let dict: [String: AnyHashable] = [:]
let mockDocument = Document(dataDict: DataDict(dict))

And because the JSONObject is still equal to [String: AnyHashable] so the next step I need to do is to add another initialization method for SelectionSet so that it can takes [String: Any] type of data for initialization, and in the method we would convert [String: Any] to JSONObject internally

@AnthonyMDev
Copy link
Contributor

Yes, that's right!

@Cookiezby Cookiezby changed the title feat: add extension for [String: Any] to transform to [String: AnyHashable] Support SelectionSet to receive [String: Any] for initialization when create mock object Oct 26, 2023
@@ -0,0 +1,38 @@
#if !COCOAPODS
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I put the new initialization method in ApolloTestSupport target to make sure this method is only used for mock purpose

@Cookiezby
Copy link
Contributor Author

@AnthonyMDev Thanks for your confirmation! I just update the code, could you help to check it?

@bdbergeron
Copy link
Contributor

As I mentioned in this comment in the apollo-ios repo, this is just a Swift compiler issue and can be solved by adding as [String: AnyHashable] to the nested dictionaries. I question whether it makes sense to add another initializer that allows for passing [String: Any] since internally Apollo needs the values to be AnyHashable.

@AnthonyMDev
Copy link
Contributor

As I mentioned in this comment in the apollo-ios repo, this is just a Swift compiler issue and can be solved by adding as [String: AnyHashable] to the nested dictionaries. I question whether it makes sense to add another initializer that allows for passing [String: Any] since internally Apollo needs the values to be AnyHashable.

Thanks for the feedback. While that is possible, I don't see any harm in adding the helper method here to make people's lives easier working with this.

Copy link
Contributor

@AnthonyMDev AnthonyMDev left a comment

Choose a reason for hiding this comment

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

I actually think we should include this right alongside the other initializer in RootSelectionSet+JSONInitializer.swift. We should probably put @_disfavoredOverload on it, to prevent ambiguity issues also.

It should also have the parameter named data just like the other initializer.

Lastly, I'm wondering if we also need to do conversions of arrays (or arrays of dictionaries ([[String: Any]]) as well.

@Cookiezby Can you test this with some data with arrays in it and add array conversion if its needed?

@bdbergeron
Copy link
Contributor

bdbergeron commented Nov 1, 2023

As I mentioned in this comment in the apollo-ios repo, this is just a Swift compiler issue and can be solved by adding as [String: AnyHashable] to the nested dictionaries. I question whether it makes sense to add another initializer that allows for passing [String: Any] since internally Apollo needs the values to be AnyHashable.

Thanks for the feedback. While that is possible, I don't see any harm in adding the helper method here to make people's lives easier working with this.

My primary concern is that this initializer swallows any non-Hashable objects a developer may pass to it, without indicating it's doing so. At a minimum I think this new initializer should throw if a value in the dictionary cannot be converted to AnyHashable.

@AnthonyMDev
Copy link
Contributor

Oh yes! That's a great point, and one I missed. I 100% agree with that.

Copy link
Contributor

@AnthonyMDev AnthonyMDev left a comment

Choose a reason for hiding this comment

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

Sorry if I wasn't clear about this! I think we should just move this method into the SelectionSet+JSONInitializer.swift` file that already exists.

Also, this implementation currently only will handle 1 dimensional arrays of objects. We need it to handle more arrays. As this is getting more complex, I'm thinking we might want to add some tests for this as well so we can verify we are handling all the different ways of doing this.

@Cookiezby Do you feel like you're still willing to work through these complications?

@@ -36,7 +38,7 @@ let data: [String: AnyHashable] = [
]
]

let model = HeroAndFriendsQuery.Data(data: DataDict(data))
let model = HeroAndFriendsQuery.Data(dict: data)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let model = HeroAndFriendsQuery.Data(dict: data)
let model = try HeroAndFriendsQuery.Data(data: data)

/// the given JSON response data.
@_disfavoredOverload
public init(
dict: [String: Any],
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
dict: [String: Any],
data: [String: Any],

/// Convert dictionary type [String: Any] to JSONObject
/// - Parameter dict: dictionary value
/// - Returns: converted JSONObject
static func convertDictToJSONObject(dict: [String: Any]) throws -> JSONObject {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
static func convertDictToJSONObject(dict: [String: Any]) throws -> JSONObject {
private static func convertDictToJSONObject(dict: [String: Any]) throws -> JSONObject {

Comment on lines 10 to 19
// Initializes a `SelectionSet` with a raw Dictionary.
///
/// We could pass dictionary object to create a mock object for GraphQL object.
///
/// - Parameters:
/// - dict: A dictionary representing a dictionary response for a GraphQL object.
/// - variables: [Optional] The operation variables that would be used to obtain
/// the given JSON response data.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Initializes a `SelectionSet` with a raw Dictionary.
///
/// We could pass dictionary object to create a mock object for GraphQL object.
///
/// - Parameters:
/// - dict: A dictionary representing a dictionary response for a GraphQL object.
/// - variables: [Optional] The operation variables that would be used to obtain
/// the given JSON response data.
/// Initializes a `SelectionSet` with a raw JSON response object.
///
/// The process of converting a JSON response into `SelectionSetData` is done by using a
/// `GraphQLExecutor` with a`GraphQLSelectionSetMapper` to parse, validate, and transform
/// the JSON response data into the format expected by `SelectionSet`.
///
/// - Parameters:
/// - data: A dictionary representing a JSON response object for a GraphQL object.
/// - variables: [Optional] The operation variables that would be used to obtain
/// the given JSON response data.

@Cookiezby
Copy link
Contributor Author

@AnthonyMDev Yes, I am willing to help with that, for the one dimensional object, I will add tests for that, and when I check the test case in the SelectionSetTests I found the nested array in some test cases, is this the more arrays that you mentions in here?

Also, this implementation currently only will handle 1 dimensional arrays of objects. We need it to handle more arrays

 let expected = try! Hero(
      data: [
        "__typename": "Human",
        "nestedList": [[]]
      ],
      variables: nil
)

@Cookiezby
Copy link
Contributor Author

@AnthonyMDev Hi, I have added the implementation for handling the array (including nested array) when initialize the SelectionSet with dictionary object, and I also added some test cases for the new initializer, can you help to review it?

Copy link
Contributor

@AnthonyMDev AnthonyMDev left a comment

Choose a reason for hiding this comment

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

This looks great to me! Thank you so much @Cookiezby for making this one happen!

@AnthonyMDev AnthonyMDev merged commit 0c88e7a into apollographql:main Nov 8, 2023
11 checks passed
BobaFetters pushed a commit that referenced this pull request Nov 8, 2023
BobaFetters pushed a commit to apollographql/apollo-ios that referenced this pull request Nov 8, 2023
BobaFetters pushed a commit that referenced this pull request Nov 8, 2023
f0c7e8e Support SelectionSet to receive [String: Any] for initialization when create mock object (#102)

git-subtree-dir: apollo-ios
git-subtree-split: f0c7e8e
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.

4 participants