Add large scale https updates with bloom filter #355
Conversation
subsymbolic
changed the title
|
I am seeing an atb test failure on CI (but not locally) which I am investigating. Other than that, this is ready for review @brindy. |
|
@brindy and I are seeing the ATB Test failures on other branches too so have ruled out it being introduced by this PR. We will investigate it separately. |
… it to be accessed on background threads
| import Foundation | ||
|
|
||
| public struct HTTPSTransientBloomFilterSpecification { | ||
| let totalEntries: Int |
There was a problem hiding this comment.
This could be Decodable which would make the JSON parsing a little easier. Can use optionals if you're worried about data missing in future versions of the file. Data in the JSON that isn't handled is ignored by default.
Core/HTTPSUpgradeParser.swift
Outdated
| } | ||
|
|
||
| static func bloomFilterSpecification(fromJSONData data: Data) -> HTTPSTransientBloomFilterSpecification? { | ||
| guard let json = try? JSONSerialization.jsonObject(with: data, options: []) else { return nil } |
There was a problem hiding this comment.
If HTTPSTransientBloomFilterSpecification was Decodable you could just use the JSONDecoder. Likewise for this whitelist.
We've got a couple of examples in the code already, though we also have examples of JSONSeralization, but I'd like us to move to the Codable pattern as our standard. I'll add a LHF task to update them.
There was a problem hiding this comment.
Good suggestion. I had considered using Codable but was refactoring existing code and didn't know we had it in the project. I feel better using the modern pattern here, thanks!
| <element name="HTTPSUpgradeSimpleDomain" positionX="-63" positionY="-18" width="128" height="60"/> | ||
| <element name="HTTPSUpgradeWildcardDomain" positionX="-54" positionY="-9" width="128" height="60"/> | ||
| <element name="HTTPSBloomFilterSpecification" positionX="-1269" positionY="-171" width="128" height="90"/> | ||
| <element name="HTTPSWhitlistedDomain" positionX="-1098" positionY="-157" width="128" height="58"/> |
There was a problem hiding this comment.
Spelling mistake HTTPSWhitlisted -> HTTPSWhitelisted
|
|
||
| import Foundation | ||
|
|
||
| public struct HTTPSTransientBloomFilterSpecification: Equatable { |
There was a problem hiding this comment.
This is starting to seem overly complex with too many layers.
Why not just dump Core Data for this and store the files on disk? Using Codable pattern and optionals vars will help with migration issues.
If the whitelist is not likely to grow too big, drop Core Data for that too.
There was a problem hiding this comment.
We discussed this and decided that it made sense to stick with CoreData over files on disk for structured data for the migration benefits.
|
Thanks for the review @brindy! |
Task/Issue URL: https://app.asana.com/0/72649045549333/721626625236431
Tech Design URL: https://app.asana.com/0/72649045549333/727813825116580
Description:
Add large scale https support:
Steps to test this PR:
TEST ONE:
TEST TWO:
TEST THREE:
TEST FOUR:
Internal references:
Software Engineering Expectations
Technical Design Template