Skip to content

Fix dropped non-string parameters types#43

Merged
stury merged 4 commits intoyahoo:masterfrom
Eli-Liebman:el/patch
Oct 20, 2022
Merged

Fix dropped non-string parameters types#43
stury merged 4 commits intoyahoo:masterfrom
Eli-Liebman:el/patch

Conversation

@Eli-Liebman
Copy link

I ran into some bugs after updating from 1.5 to 1.6 and I tracked it down to a change that was made when converting the Objective-C version of the TDOAuth class to Swift.

Objective C:

 if (unencodedParameters != nil) {
        queryItems = [NSMutableArray new];
        for (NSString *key in unencodedParameters.allKeys) {
            TDOQueryItem *queryItem = [TDOQueryItem itemWithName:key value:unencodedParameters[key]];
            [queryItems addObject:queryItem];
        }
    }

Swift:

if let unencodedParameters = unencodedParameters {
           for key in unencodedParameters.keys {
               if let key = key as? String,
                   let value = unencodedParameters[key] as? String {
                   let queryItem = TDOQueryItem(name: key, value: value)
                   queryItems.append(queryItem)
               }
           }
       }

The let value = unencodedParameters[key] as? String in the Swift version causes any value type that isn't a String to be ignored and dropped. My particular issue was losing my integer parameters but I added Bool handling while I was in there.

I think that should be enough but can add more if needed.

Copy link
Collaborator

@stury stury left a comment

Choose a reason for hiding this comment

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

Thanks @Eli-Liebman !

@stury stury merged commit 2bb9947 into yahoo:master Oct 20, 2022
@adamkaplan
Copy link
Contributor

Nice find. That must have been frustrating..

Did you consider if there’s a way to catch all with this protocol? https://developer.apple.com/documentation/swift/string/init(_:)-1ywfq

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