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

[feat] Add support for custom URL parameters percent encoding #56

Merged
merged 8 commits into from
Feb 26, 2024

Conversation

matejmolnar
Copy link
Collaborator

@matejmolnar matejmolnar commented Dec 12, 2023

This PR is trying to solve issue #55

Changes

Introduce a new type PercentEncodedParameter which can be passed as Any in urlParameters: [String: Any]. The value of this parameter won't get encoded during the URLRequest creation.

@matejmolnar matejmolnar requested a review from a team December 12, 2023 11:20
johnkodes
johnkodes previously approved these changes Dec 15, 2023
Sources/Networking/Core/RequestURLParametersType.swift Outdated Show resolved Hide resolved
Sources/Networking/Core/Requestable+Convenience.swift Outdated Show resolved Hide resolved
Sources/Networking/Core/Requestable+Convenience.swift Outdated Show resolved Hide resolved
@cejanen
Copy link
Collaborator

cejanen commented Dec 29, 2023

@matejmolnar I played with it for a bit and I would propose a bit difference solution. Custom string encoding similar like specific array encoding is edge case. In 99% cases user is ok with parameters [String: Any]. Therefore I would not push them to wrap always parameters into RequestURLParametersType. I would better go with similar approach like we have for ArrayParameter.
We can do CustomEncodingParameter with either string which will be encoded for URL automatically or allow already encoded values.
Code which may help (draft):

if let percentedEncoded = value as? PercentEncodedParameter {
            queryItems.append(
                URLQueryItem(
                    name: key,
                    value: percentedEncoded.value
                )
                .percentEncodedWithValue(string: percentedEncoded.value)
            )
            return queryItems
        }
    
extension URLQueryItem {
    func percentEncoded() -> URLQueryItem {
        /// addingPercentEncoding(withAllowedCharacters: .urlQueryAllowed) encode parameters following RFC 3986
        /// which we need to encode other special characters correctly.
        /// We then also encode "+" sign with its HTTP equivalent

        var newQueryItem = self
        newQueryItem.value = value?
            .addingPercentEncoding(withAllowedCharacters: .urlQueryAllowed)?
            .replacingOccurrences(of: "+", with: "%2B")

        return newQueryItem
    }
    
    func percentEncodedWithValue(string: String) -> URLQueryItem {
        var newQueryItem = self
        newQueryItem.value = string
        return newQueryItem
    }
}

public struct PercentEncodedParameter {
    let value: String
    
    public init(_ value: String) {
        self.value = value
    }
}

@gajddo00
Copy link
Contributor

gajddo00 commented Jan 1, 2024

@cejanen Agree, the intention behind designing ArrayParameter as it is was to avoid publishing a new breaking change to the library and making it optional for the user at the same time. For context, this is the Requestable+Convenience query building implementation, I'm also adding an adjusted sample from CJs draft if it makes sense.

// MARK: Build Query Items
private extension Requestable {
  func buildQueryItems(urlParameters: [String: Any]) -> [URLQueryItem] { ... }

  func buildQueryItems(key: String, value: Any) -> [URLQueryItem] {
      switch value {
      case let parameter as ArrayParameter:
          return buildArrayParameter(key: key, parameter: parameter)
       case let parameter as PercentEncodedParameter:
           return buildPercentEncodedParameter(key: key, parameter: parameter)
       default:
           return [URLQueryItem(name: key, value: String(describing: value))]
      }
  }
  
   func buildArrayParameter(
        key: String,
        parameter: ArrayParameter
    ) -> [URLQueryItem] { ... }
    
    func buildPercentEncodedParameter(
        key: String,
        parameter: PercentEncodedParameter
    ) -> [URLQueryItem] {
        [URLQueryItem(name: key, value: parameter.percentEncodedValue)]
    }
}

public struct PercentEncodedParameter {
    let value: String

    public var percentEncodedValue: String? {
        value.addingPercentEncoding(withAllowedCharacters: .urlQueryAllowed)?
            .replacingOccurrences(of: "+", with: "%2B")
    }

    public init(value: String) {
        self.value = value
    }
}

is 'order to give a user the flexibility to encode the parameters however they want' this meant as the user can decide which parameters should be percent encoded or use literally any type of encoding for any parameter? then we'd be solving a different problem

@matejmolnar matejmolnar force-pushed the fix/55-url-encoding-and-the-+-sign branch from 404e151 to 4f041ff Compare January 2, 2024 07:56
@matejmolnar
Copy link
Collaborator Author

@cejanen @gajddo00 Thank you for the suggestions 🙏, I agree that it's a better approach although I had to adjust it a little.

gajddo00
gajddo00 previously approved these changes Jan 2, 2024
Copy link
Contributor

@gajddo00 gajddo00 left a comment

Choose a reason for hiding this comment

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

Looks good to me 👍

Copy link
Collaborator

@cejanen cejanen left a comment

Choose a reason for hiding this comment

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

I tested it and it doesn't work as expected. Will try to do updates. It wasn't very clear from my side.

tested URL params
page=1&test=1+1

@cejanen
Copy link
Collaborator

cejanen commented Feb 21, 2024

@gajddo00 @matejmolnar At the end I decided for simplest solution. By default are data encoded on URLComponent and you can pass custom encoded data which are expected like encoded already. All tests pass multiple times.

Comment on lines +10 to +17
public extension String {
/// Help method to allow custom + sign encoding, more in ```CustomEncodedParameter```
func plusSignEncoded() -> Self? {
self
.addingPercentEncoding(withAllowedCharacters: .urlQueryAllowed)?
.replacingOccurrences(of: "+", with: "%2B")
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we could just move this extension to the URLParametersTests file, since it's not being used anywhere else 🤔

/// // Request URL "https://test.com?specialCharacter=%3E"
///
/// var urlParameters: [String: Any]? {
/// ["specialCharacter": PercentEncodedParameter(">")]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Suggested change
/// ["specialCharacter": PercentEncodedParameter(">")]
/// ["specialCharacter": CustomEncodedParameter(">")]

Copy link
Contributor

@gajddo00 gajddo00 left a comment

Choose a reason for hiding this comment

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

🍓 Agreed as in most cases custom encoding won't be necessary, we can elevate this to the user, why not.

@cejanen cejanen merged commit effc8b7 into dev Feb 26, 2024
@cejanen cejanen deleted the fix/55-url-encoding-and-the-+-sign branch February 26, 2024 09:59
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