Skip to content
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

FirestoreDbBuilder.ConverterRegistry ignored when using Firestore Emulator #6313

Closed
jrojasledesma opened this issue Apr 5, 2021 · 5 comments · Fixed by #6314
Closed

FirestoreDbBuilder.ConverterRegistry ignored when using Firestore Emulator #6313

jrojasledesma opened this issue Apr 5, 2021 · 5 comments · Fixed by #6314
Assignees
Labels
api: firestore Issues related to the Firestore API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@jrojasledesma
Copy link

Hi. I'm having problems trying to use custom type converters via the Converter Registry in FirestoreDbBuilder when targeting the Firestore Emulator.

Looking a the source code I noticed that FirestoreDbBuilder ignores the value of its ConverterRegistry property when creating an instance of FirestoreDb. The problem seems to be in lines 186-192 of FirestoreDbBuilder.cs, inside the function MaybeUseEmulator:

...
 return new FirestoreDbBuilder
{
    Endpoint = hostAndPort,
    Settings = settings,
    ProjectId = ProjectId,
    ChannelCredentials = ChannelCredentials.Insecure
};

Note that in the new FirestoreDbBuilder returned by this method, ConverterRegistry is not asigned. Thus, the original value is ignored when the FirestoreDb instance is created in the recursive call to Build in line 89 of FirestoreDbBuilder.cs.

Is there any good reason to ignore the value, or is this simply a bug?

@jskeet
Copy link
Collaborator

jskeet commented Apr 5, 2021

I'll have a look when I'm back at work tomorrow. It's almost certainly just a bug.

@jskeet jskeet self-assigned this Apr 5, 2021
@jskeet jskeet added api: firestore Issues related to the Firestore API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Apr 5, 2021
@jskeet
Copy link
Collaborator

jskeet commented Apr 5, 2021

(I suspect it will be a bug that's easily fixed, and which we can then just roll out a patch release for. But we'll see...)

@jrojasledesma
Copy link
Author

Thanks! Good to know this could be fixed soon.

jskeet added a commit to jskeet/google-cloud-dotnet that referenced this issue Apr 6, 2021
jskeet added a commit to jskeet/google-cloud-dotnet that referenced this issue Apr 6, 2021
Changes in this release:

- [Commit 62182ca](googleapis@62182ca): fix: Propagate properties in FirestoreDbBuilder when using the emulator. Fixes [issue 6313](googleapis#6313)

(No API surface change, as expected in a patch release.)
jskeet added a commit that referenced this issue Apr 6, 2021
Changes in this release:

- [Commit 62182ca](62182ca): fix: Propagate properties in FirestoreDbBuilder when using the emulator. Fixes [issue 6313](#6313)

(No API surface change, as expected in a patch release.)
@jskeet
Copy link
Collaborator

jskeet commented Apr 6, 2021

Hi @jrojasledesma, the fix is now released in version 2.3.1. I'm pretty confident in the fix, but please shout if you have any problems :)

@jrojasledesma
Copy link
Author

Hi, I just tried the new version and confirmed that my issue is now fixed. Thank you @jskeet and @amanda-tarafa!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: firestore Issues related to the Firestore API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants