Firestore: Make @DocumentID value setter private #10167

andrewheard · 2022-09-02T23:41:51Z

Updated the DocumentID Swift property wrapper's value property to be private(set) since it is ignored in the methods that write to Firestore.

Existing Implementation (See issue #9368)

struct User: Codable {
  @DocumentID var id: String?
  var name: String
}

var user = User(name: "Foo")
user.id = "abc123"  // This line is ignored

try! Firestore.firestore().collection("users").document("def456").setData(from: user)
// User is written to "users/def456", `user.id` is ignored.

Updated Implementation

// Compiler Error: Cannot assign to property: 'id' is a get-only property
user.id = "abc123"

API

This could be considered an API change and may need to go through review

Updated the `DocumentID` Swift property wrapper's `value` property to be private(set) since it is ignored in the methods that write to Firestore.

paulb777 · 2022-09-02T23:47:27Z

Also may considered a breaking change. Adding @peterfriese for input

Firestore/Swift/Source/Codable/DocumentID.swift

peterfriese

I am generally in favour of this, since it makes the current behaviour explicit. To my knowledge, there is no way to ever change a Firestore document ID.

I'd like to suggest to document this behaviour at a suitable place on the property wrapper.

Also, I am not sure if it's possible to write a test for this?

mortenbekditlevsen · 2022-09-03T08:23:00Z

It's true that you can't change an id, but you could expect an initial write to use a manually set document id. This, however, is not how the api works, so making it read only will clarify any confusion one might have had about that behavior.

Firestore/Swift/Source/Codable/DocumentID.swift

paulb777 · 2022-09-16T23:38:47Z

LGTM, adding @chliangGoogle for API review.

Made the setter internally accessible and imported FirebaseFirestoreSwift with @testable.

andrewheard · 2022-09-21T01:08:56Z

I'd like to suggest to document this behaviour at a suitable place on the property wrapper.

Also, I am not sure if it's possible to write a test for this?

@peterfriese I've attempted to add some more documentation to the property wrapper to clarify the usage. I'm not sure that it's possible to write a test for this, beyond the existing @DocumentId usage tests, because they'd need to be non-compile tests to verify that setting isn't possible. I've only done that manually.

andrewheard · 2022-09-21T01:11:21Z

It's true that you can't change an id, but you could expect an initial write to use a manually set document id. This, however, is not how the api works, so making it read only will clarify any confusion one might have had about that behavior.

@mortenbekditlevsen That's helpful feedback though and we'll want to keep it in mind when designing APIs in the future.

google-oss-bot · 2022-09-21T01:56:34Z

Coverage Report ¹

Affected Products

FirebaseFirestore-iOS-FirebaseFirestore.framework
Overall coverage changed from 87.93% (a6164a8) to 87.91% (21efad9) by -0.02%.
Filename Base (a6164a8) Merge (21efad9) Diff
leveldb_key.cc 98.33% 98.14% -0.20%
leveldb_remote_document_cache.cc 96.25% 94.38% -1.88%
FirebaseFirestore-iOS-FirebaseFirestoreSwift.framework
Overall coverage changed from 45.67% (a6164a8) to 45.50% (21efad9) by -0.17%.
Filename Base (a6164a8) Merge (21efad9) Diff
DocumentID.swift 87.88% 80.56% -7.32%

Test Logs

https://storage.googleapis.com/firebase-sdk-metric-reports/svP2VPx1eF.html

# Conflicts: # Firestore/Swift/CHANGELOG.md # Firestore/Swift/Source/Codable/DocumentID.swift

Unblock for merge today

#10275) * [Firestore] Mark `@DocumentID` setter deprecated and add log statement in init * Fix typo in import * Update CHANGELOG * Review and remove deprecation * Review and add FIRLogWarningSwift * Add FirebaseCorExtension to Firestore's Podfile

ncooke3 · 2022-09-28T05:38:03Z

Merged associated PRs and resolved simple conflicts in the last two commits. Proceeding with merge. Will move release tags, stage FirebaseFirestoreSwift.podspec (and Analytics specs!), and kick off zip build.

ncooke3 · 2022-09-28T07:25:58Z

Zip build in progress at https://github.com/firebase/firebase-ios-sdk/actions/runs/3141588403

peterfriese · 2022-09-28T10:52:16Z

Firestore/Swift/Source/Codable/DocumentID.swift

+      "[FirebaseFirestoreSwift]",
+      "I-FST000002",
+      """
+      Attempting to initialize or set a @DocumentID property with a non-nil


Breaking the error message like this results in the following, which is less than ideal:

2022-09-28 12:49:44.813075+0200 Make It So[29167:1864911] 10.0.0 - [FirebaseFirestoreSwift][I-FST000002] Attempting to initialize or set a @DocumentID property with a non-nil value: B89FE0DD-7ACB-4D55-9FBE-CFC70512E760. The document ID is managed by Firestore and any initialized or set value will be ignored. The ID is automatically set when reading from Firestore."

Can we make sure the error message ends up being a single line? It also seems like there is a stray quotation mark at the end.

Thanks @peterfriese, if you don't mind and your app still running... Could you suggest the formatted string that renders it better?

Sure! In a comment, or rather on a follow-up PR?

Let's do a follow-up PR!

Sorry I didn't reply in time. This looks good to me on the console:

message: "Attempting to initialize or set a @DocumentID property with a non-nil value:" + " \"\(value)\". The document ID is managed by Firestore and any initialized or set value" + " will be ignored. The ID is automatically set when reading from Firestore."

I'm hoping the compiler will optimize it into one long string but it may do concatenation at runtime, not sure.

I've found a way to use multi-line strings, see #10289

peterfriese · 2022-09-28T12:41:15Z

Firestore/Swift/Source/Codable/DocumentID.swift

    get { value }
-    set { value = newValue }
+    set {
+      if let someNewValue = newValue {


Is there a reason why we use someNewValue here? In line 111, we use if let value = value.

So I recall being confused about introducing another newValue into the scope. Mainly because I didn't know if it could cause problems on L123. Swift should be able to understand that the non-optional newValue is used in the unwrapped scope and the optional newValue should be set in the outer scope on L123. But I figured it was possible that an older Swift version may not know. To be extra safe (it was getting late when I worked on this 😅 ), I opted for introducing a new name in the scope.

I guess in Swift 5.7, we should even be able to use

if let newValue { }

I don't think we can use this feature yet since we still need to build for devs using Xcode 13.3.1 x Swift 5.6.

Just verified my assumption.

Also, wrt the below #10167 (comment), the setter should log if you did something like:

struct Model: Codable { @DocumentID var id: String = "hello" } let model = // ... model.id = nil // ⚠️ Logs a warning ⚠️

Firestore/Swift/Source/Codable/DocumentID.swift

Firestore: Make @documentid value setter private

0cbe264

Updated the `DocumentID` Swift property wrapper's `value` property to be private(set) since it is ignored in the methods that write to Firestore.

andrewheard self-assigned this Sep 2, 2022

andrewheard linked an issue Sep 2, 2022 that may be closed by this pull request

FirestoreSwift Library: Setting a @DocumentId fails all future saves #9368

Closed

andrewheard added the api: firestore label Sep 2, 2022

andrewheard requested a review from paulb777 September 2, 2022 23:43

paulb777 requested a review from peterfriese September 2, 2022 23:47

peterfriese reviewed Sep 3, 2022

View reviewed changes

Firestore/Swift/Source/Codable/DocumentID.swift Outdated Show resolved Hide resolved

peterfriese previously requested changes Sep 3, 2022

View reviewed changes

paulb777 added this to the Firebase 10 - M122 milestone Sep 8, 2022

andrewheard added 2 commits September 15, 2022 16:21

Make initializer private

05f6332

Fix formatting

8fadb0d

paulb777 reviewed Sep 15, 2022

View reviewed changes

Firestore/Swift/Source/Codable/DocumentID.swift Outdated Show resolved Hide resolved

paulb777 requested a review from charlotteliang September 16, 2022 23:38

Fix tests

ae9d033

Made the setter internally accessible and imported FirebaseFirestoreSwift with @testable.

Add a changelog entry and formatting fixes

39eacbc

andrewheard added 2 commits September 27, 2022 12:45

Merge branch 'master' into ah-firestore-documentid

7125162

# Conflicts: # Firestore/Swift/CHANGELOG.md # Firestore/Swift/Source/Codable/DocumentID.swift

Documentation fixes

d963cd7

ncooke3 approved these changes Sep 28, 2022

View reviewed changes

paulb777 approved these changes Sep 28, 2022

View reviewed changes

ncooke3 and others added 3 commits September 28, 2022 01:14

[skip ci] Merge with master and resolve conflicts

0254cae

[skip ci] Fix merged comment

b4036a5

ncooke3 merged commit 8866b22 into master Sep 28, 2022

ncooke3 deleted the ah-firestore-documentid branch September 28, 2022 05:42

This was referenced Sep 28, 2022

[Firestore] Update CHANGELOG for new Count API (#10246) #10278

Merged

Nightly Testing Report #10280

Closed

peterfriese reviewed Sep 29, 2022

View reviewed changes

peterfriese mentioned this pull request Sep 29, 2022

DocumentId property wrapper: Emit error message on a single line #10289

Merged

This was referenced Oct 5, 2022

FirestoreSwift: Reword DocumentID breaking change in changelog #10305

Merged

[FirestoreSwift] Add FirebaseCore dependency to avoid linker issues #10319

Merged

firebase locked and limited conversation to collaborators Oct 29, 2022

Firestore: Make @DocumentID value setter private #10167

Firestore: Make @DocumentID value setter private #10167

Uh oh!

Conversation

andrewheard commented Sep 2, 2022

Existing Implementation (See issue #9368)

Updated Implementation

API

Uh oh!

paulb777 commented Sep 2, 2022

Uh oh!

Uh oh!

peterfriese left a comment

Choose a reason for hiding this comment

Uh oh!

mortenbekditlevsen commented Sep 3, 2022

Uh oh!

Uh oh!

paulb777 commented Sep 16, 2022

Uh oh!

andrewheard commented Sep 21, 2022

Uh oh!

andrewheard commented Sep 21, 2022

Uh oh!

google-oss-bot commented Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report 1

Affected Products

FirebaseFirestore-iOS-FirebaseFirestore.framework

FirebaseFirestore-iOS-FirebaseFirestoreSwift.framework

Test Logs

Uh oh!

ncooke3 commented Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ncooke3 commented Sep 28, 2022

Uh oh!

peterfriese Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ncooke3 Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

google-oss-bot commented Sep 21, 2022 •

edited

Loading

Coverage Report ¹

`FirebaseFirestore-iOS-FirebaseFirestore.framework`

`FirebaseFirestore-iOS-FirebaseFirestoreSwift.framework`

ncooke3 commented Sep 28, 2022 •

edited

Loading

peterfriese Sep 28, 2022 •

edited

Loading

ncooke3 Sep 29, 2022 •

edited

Loading