-
Notifications
You must be signed in to change notification settings - Fork 428
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
[Bug] Annoying warning when converting from a DocumentSnapshot to a class. #1144
Comments
@cgascons Can you clarify if you're using Realtime Database or Firestore? You've specified "database" but the code looks like Firestore. Could you update your code sample to include any relevant "using" declarations? That would also help disambiguate. Oh, and could you include the definition of the |
@dconeybe Sorry, you are right, we are using Firestore db. |
Ok, we have been able to create a standalone project and have uploaded it to this wetransfer link. |
@cgascons Thank you for providing the reproduction app! I apologize for the delay but I haven't had a chance to try it out yet. I'm hoping to test it out early next week. |
Ok I was easily able to reproduce this warning based on your sample project. Thank you for providing that! Here is the code that I used: using Firebase;
using Firebase.Extensions;
using Firebase.Firestore;
using System.Collections;
using UnityEngine;
[FirestoreData]
public class PlayerData : ScriptableObject {
[FirestoreProperty]
public string m_Id { get; set; }
[FirestoreProperty]
public string m_Email { get; set; }
}
public class UnityBug1144 : MonoBehaviour {
IEnumerator Start() {
{
var task = FirebaseApp.CheckAndFixDependenciesAsync();
yield return new WaitUntil(() => task.IsCompleted);
if (task.Exception != null) {
Debug.LogError("CheckAndFixDependenciesAsync() failed: " task.Exception);
}
}
FirebaseFirestore firestore = FirebaseFirestore.DefaultInstance;
CollectionReference collection = firestore.Collection("UnityBug1144");
DocumentReference doc = collection.Document("player");
DocumentSnapshot snapshot = null;
{
var task = doc.GetSnapshotAsync();
yield return new WaitUntil(() => task.IsCompleted);
if (task.Exception != null) {
Debug.LogError("GetSnapshotAsync() failed: " task.Exception);
}
snapshot = task.Result;
}
if (! snapshot.Exists) {
var newPlayer = ScriptableObject.CreateInstance<PlayerData>();
newPlayer.m_Id = "PlayerId";
newPlayer.m_Email = "[email protected]";
var task = doc.SetAsync(newPlayer);
yield return new WaitUntil(() => task.IsCompleted);
if (task.Exception != null) {
Debug.LogError("SetAsync() failed: " task.Exception);
}
} else {
PlayerData existingPlayer = snapshot.ConvertTo<PlayerData>();
Debug.Log("m_Id: " existingPlayer.m_Id);
Debug.Log("m_Email: " existingPlayer.m_Email);
}
}
} And here is the warning that resulted:
|
So the problem is that In this case, this is not the correct way to create a new instance. Off the top of my head, I can think of two solutions to this problem:
What are your thoughts on these approaches? I'll also discuss with my team for other ideas. |
Hi @dconeybe, thanks for your reply, I will test the two workarounds you provided and will let you know the results |
@cgascons Sorry if I was unclear, those are not "workarounds". They are proposed changes to the Firestore Unity SDK to accommodate your use case, and I was interested in your thoughts on them. The only "fix" or "workaround" for your issue today would be to remove the |
Hi @dconeybe, sure now that I read your post in detail I see that you were suggesting changes to the SDK, sorry about that. As far as I understand, both suggested approaches would require manual work, right? At the top of my head I believe we would have to do the following steps for either solution:
If I understood this right that would mean that we would have to manually loop through all the DocumentSnapshot attributes which is what we were trying to avoid in the first place so it wouldn't be an ideal solution (at least to us). |
Hey @dconeybe any news regarding this? |
Hi @cgascons. We discussed this as a team and our latest idea is to add support for a new annotation to support the desired behavior. At the moment we've named it [FirestoreData]
public class PlayerData : ScriptableObject {
[FirestoreProperty]
public string m_Id { get; set; }
...
// This is an example of the proposed annotation
[FirestoreDataInstanceCreator]
public static PlayerData NewInstance() {
return ScriptableObject.CreateInstance<PlayerData>();
}
} If present, then Unfortunately, I cannot provide an ETA for this feature to land, but I would hope for Jan-Feb 2022 at the earliest based on our competing priorities and planned release schedule. I'm sorry that I don't better news for you! In the meantime this is something that you will need to work around. |
I see, thank you for the info anyways, will wait till the feature is released. |
I will update this GitHub Issue once the feature is released. |
Hello, I am trying to use derived classes for Firestore Unity and got stuck.
When I: document.ConvertTo< SchdeuleRecord >() is there any way I can convert RoundRecord to if there were any overloading method on creation, it would be great. |
@TaejunPark. Would you mind opening a new issue with your question? I want to avoid re-purposing this thread. |
[REQUIRED] Please fill in the following fields:
[REQUIRED] Please describe the issue here:
We are performing a very simple query which gets from the Firestore database a document, which comes in a DocumentSnapshot format and we are trying to convert it to a ScriptableObject, everything works just right, but there's this annoying warning in the Unity console which we can't get rid of and we were wondering if there's anything we might be doing wrong.
Steps to reproduce:
Have you been able to reproduce this issue with just the Firebase Unity quickstarts (this GitHub project)? No
What's the issue repro rate? 100%
What happened?
Log trace:
BidInstanceData must be instantiated using the ScriptableObject.CreateInstance method instead of new BidInstanceData.
How can we make the problem occur?
Yes, just retrieve a document from the database and try to convert it to a ScriptableObject.
Relevant Code:
The text was updated successfully, but these errors were encountered: