Authv2 / Networking improvements #1168
Conversation
# Conflicts: # Package.swift
federicocappelli
requested review from
a team,
studiosutara and
alessandroboron
and removed request for
a team and
studiosutara
| import Foundation | ||
| import Common | ||
|
|
||
| extension Dictionary where Key == String, Value == String { |
There was a problem hiding this comment.
can we make it a Collection extension defaulting to KeyValuePairs<String, String> – this will preserve items order
There was a problem hiding this comment.
Good suggestion thanks, I agree that maintaining the order is a nice to have, I tried to change it in
public typealias QueryItems = any Collection<KeyValuePairs<String, String>>
But that introduces a lot of issues (like can't access values directly by key), then I decided to stuck with
public typealias QueryItem = Dictionary<String, String>.Element
public typealias QueryItems = Array<QueryItem>
Less elegant but achieves the objective of having a data structure of ordered key value pairs
I moved everything to QueryItems.swift
| } | ||
| urlComps.queryItems = queryItems?.toURLQueryItems(allowedReservedCharacters: allowedQueryReservedCharacters) |
There was a problem hiding this comment.
why are we overwriting existing url components with passed query items, is it intended? It seems not intuitive
There was a problem hiding this comment.
Correct, if a URL already containing query items was passed this would have deleted them, case that I never imagined. Fixed thanks.
| QueryParameter.category: configuration.threatKind.rawValue, | ||
| QueryParameter.revision: (configuration.revision ?? 0).description, | ||
| ]) | ||
| return [QueryParameter.category: configuration.threatKind.rawValue, |
There was a problem hiding this comment.
addition to my question for APIRequestV2 change, why do we need to overwrite url components with an extra query items parameter? the old url(for:requestType) allowed any url configuration in 1 place, now we need an extra method for it to work. Also it‘s doing url encoding/decoding several times back and forth which seems suboptimal.
I‘d keep an old implementation allowing to combine url items and passed items unless it‘s really needed. Or at least provide some in-code comments why it must be this way and probably add some assertions validating there‘s no URL query items overwritten.
There was a problem hiding this comment.
In the new Subscription code we don't build the URL before creating the API request, everything happens inside the init, I tried to use the same approach here but I made to many chanhges. As per your previous comment now the APiRequestV2 joins the QueryParams passed and the one already present in the URL, this allowed me to reverte these changes.
Task/Issue URL: https://app.asana.com/0/1205842942115003/1209170372758735/f
iOS PR: duckduckgo/iOS#3820
macOS PR: duckduckgo/macos-browser#3746
What kind of version bump will this require?: Major (APIRequestV2 init can now return nil, TestUtils library has been replaced)
CC: @miasma13
Description:
Networking v2 Improvements
NetworkingTestingUtilsAdditional changes include:
DecodableHelperexpanded and renamedCodableHelperDateextension with utilities + unit testsXYZTestingUtilsmodule, I removed the genericTestingUtilsSteps to test this PR:
Internal references:
Software Engineering Expectations
Technical Design Template