User defaults

[manjanac]

Hey there! So you want to contribute to a Firebase SDK?
Before you file this pull request, please read these guidelines:

Discussion

• Read the contribution guidelines (CONTRIBUTING.md).
• If this has been discussed in an issue, make sure to link to the issue here.
  If not, go file an issue about this before creating a pull request to discuss.

Testing

• Make sure all existing tests in the repository pass after your change.
• If you fixed a bug or added a feature, add a new test to cover your code.

API Changes

• At this time we cannot accept changes that affect the public API. If you'd like to help
  us make Firebase APIs better, please propose your change in a feature request so that we
  can discuss it together.

[google-oss-bot]

	1 Warning
⚠️	Did you forget to add a changelog entry? (Add #no-changelog to the PR description to silence this warning.)

Generated by 🚫 Danger

[manjanac]

What I'm checking here is if newValue is nil - is this the right way to do that?

[maksymmalyhin]

I wonder if something like the following will work:

if let newValue = newValue else { 
  storage.setValue(newValue, forKey: key)
} else {
  storage.removeObject(forKey: key)
}

[ryanwilson]

I don't think this will work. I was playing around in a playground and remember seeing something like this before and came up with a different (somewhat messy?) solution:

private protocol OptionalProtocol: ExpressibleByNilLiteral {
  var isNil: Bool { get }
}

extension Optional: OptionalProtocol {
  public var isNil: Bool {
    switch self {
    case .some(_): return false
    case .none: return true
    }
  }
}

This is just saying that there's a protocol that nil "conforms" to and Optional does as well, then the check can be against the protocol:

if let opt = newValue as? OptionalProtocol, opt.isNil {
  // Value is nil, remove it.
} else {
  // Value is not nil.
}

Does that make sense? It's not the nicest but it works as far as I can tell :P

[manjanac]

Makes sense, I updated the code to look more like this.

[manjanac]

I figured this line would work, given that getTestInstance() wipes out user defaults, but it fails. What am I missing?

[manjanac]

@maksymmalyhin

[ryanwilson]

Please move this to a new file since it could be used elsewhere. Could also eventually be pulled out into a common utility for other products as well.

[manjanac]

Done.

[ryanwilson]

I don't think this will work. I was playing around in a playground and remember seeing something like this before and came up with a different (somewhat messy?) solution:

private protocol OptionalProtocol: ExpressibleByNilLiteral {
  var isNil: Bool { get }
}

extension Optional: OptionalProtocol {
  public var isNil: Bool {
    switch self {
    case .some(_): return false
    case .none: return true
    }
  }
}

This is just saying that there's a protocol that nil "conforms" to and Optional does as well, then the check can be against the protocol:

if let opt = newValue as? OptionalProtocol, opt.isNil {
  // Value is nil, remove it.
} else {
  // Value is not nil.
}

Does that make sense? It's not the nicest but it works as far as I can tell :P

[maksymmalyhin]

nit: completion(...) on a new line?

[manjanac]

Done.

[maksymmalyhin]

Would it make sense to have a separate guard to unwrap self at the beginning of the handler?

[maksymmalyhin]

I wonder why completion is not called in this case? Could you add a comment please?

[manjanac]

My bad, I fixed it. The error handling needs to be fleshed out in a future PR.

[maksymmalyhin]

Should we unwrap self at the beginning of the handler?

[manjanac]

Done.

[maksymmalyhin]

nit: will it be more readable if we move sending the request to a separate method?

[manjanac]

True. I've added a TODO for myself to do this in a future PR, since this one is primarily about user defaults and FIS.

[maksymmalyhin]

Standard caching headers like ETag are usually handled transparently by NSURLSession. I wonder what's the reason of additional manual handling?

[manjanac]

I use ETag to extract the model hash returned by the server for the model object.

[maksymmalyhin]

nit: It may be better to express this using switch with pattern matching, e.g.:

switch httpResponse.statusCode {
case 100..<300:
  // Handle success codes
  break
case 304:
  return

default:
...
}

In this case it will be easier to spot handled and unhandled cases.

[manjanac]

Done.

[maksymmalyhin]

I wonder if something like the following will work:

if let newValue = newValue else { 
  storage.setValue(newValue, forKey: key)
} else {
  storage.removeObject(forKey: key)
}

[maksymmalyhin]

Please avoid using force unwrap in the production code unless it is 100% safe or required as it potentially leads to unexpected obfuscated crashes.

[manjanac]

I looked into this and it seems like UserDefaults will only fail when you pass it a reserved (therefore invalid) suiteName. So technically this should work 100% of the time, but I've added a TODO here to look into this more and add a test in a future PR.

[maksymmalyhin]

From my experience, we should rely only on the public API definition as the implementation details may change without a notice, so I'm pretty convinced we should handle nullability gracefully here.

[manjanac]

Sounds good. I've added a TODO to handle this in a future PR.

[maksymmalyhin]

I would try to avoid adding test code to production version. Can it be defined in tests instead?

[manjanac]

Done.

[maksymmalyhin]

Mostly LGTM for an intermediate PR, but a couple things to be addressed here or later on.

[maksymmalyhin]

I enjoy seeing how a property wrapper can extend functionality of a simple class, it looks really cool. From the other side I'm a bit worried of the fact that the struct that looks like a simple data object actually has a very significant side effect and modifies a shared global state (user defaults). As it is a value type, a new copy of the model will be created each time the model is modified but all of these copies will actually modify the same user defaults affecting other copies violating the value type semantic.

Do you think I'm missing anything? If not then I think there are a couple ways to proceed:

1. accept the controversy of a data object modifying the global shared state in favour the convenience. In this case the model should be redefined as a class to fix conflict between the value type semantic and actual model behaviour
2. separate the model persistence logic and make ModelInfo a plain data value type.

I think we may address it in a followup PR just to keep moving. If you decide to do so please add a TODO for it.

[manjanac]

Added a TODO, and for now ModelInfo is a class.

[maksymmalyhin]

nit: the String type declaration is redundant when assigned with the literal here and below. It should be just:

static let fisTokenHTTPHeader = "x-goog-firebase-installations-auth"

[manjanac]

Done.

[maksymmalyhin]

No force unwrap should be used in production code.

[manjanac]

I've added a TODO to handle this in a future PR.

[maksymmalyhin]

If I am not mistaken URLSession.shared caches request and response into an unencrypted database in the app sandbox, which for some cases may be considered a security vulnerability when request contains data like FIS token API key, etc. You may find more details in b/158644295.

Could you please add a TODO to reconsider using the shared URL session for the follow up PRs.

[maksymmalyhin]

From my experience, we should rely only on the public API definition as the implementation details may change without a notice, so I'm pretty convinced we should handle nullability gracefully here.

[paulb777]

This looks problematic and looks to be causing CI failure