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

Fixing Database emulation #278

Merged
merged 8 commits into from Dec 7, 2021
Merged

Fixing Database emulation #278

merged 8 commits into from Dec 7, 2021

Conversation

LeonardMeagher2
Copy link
Contributor

@LeonardMeagher2 LeonardMeagher2 commented Dec 7, 2021

I've added comments for all my code changes to better explain them.

Overall

  • Make auth_request signal values match expected results
  • fix database emulation + workaround for events
  • some small cleanup things

Related issues:
(firebase/firebase-tools#3329)
#154

@@ -379,7 +379,7 @@ func _on_FirebaseAuth_request_completed(result : int, response_code : int, heade
var json_result = JSON.parse(bod)
if json_result.error != OK:
Firebase._printerr("Error while parsing auth body json")
emit_signal("auth_request", "Error while parsing auth body json", json_result)
emit_signal("auth_request", ERR_PARSE_ERROR, "Error while parsing auth body json")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +393 to +394
# Refresh token countdown
emit_signal("auth_request", 1, auth)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A token refresh is not an error, and 1 is expected on success

Comment on lines +23 to +29
_base_url = _config.databaseURL
else:
var port : String = _config.emulators.ports.realtimeDatabase
if port == "":
Firebase._printerr("You are in 'emulated' mode, but the port for Realtime Database has not been configured.")
else:
_base_url = "http://localhost:{port}/?ns={projectId}".format({ port = port, projectId = _config.projectId }) + "/"
_base_url = "http://localhost"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved the addition of the separator to the reference.gd
Also when using connect_to_host the domain cannot contain path content (like a query string or port)

Comment on lines +74 to +77
var port = -1
if Firebase.emulating:
port = int(_config.emulators.ports.realtimeDatabase)
_listener.connect_to_host(base_url, extended_url, port)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-1 is the default value
The port must be passed as an additional parameter when using connect_to_host

Comment on lines +145 to +146
if Firebase.emulating:
remaining_path += "&ns="+_config.projectId+"-default-rtdb"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When emulating a namespace must be specified, it is your projectId + the extra content there.

Comment on lines +150 to +154
func _get_list_url(with_port:bool = true) -> String:
var url = Firebase.Database._base_url.trim_suffix(_separator)
if with_port and Firebase.emulating:
url += ":" + _config.emulators.ports.realtimeDatabase
return url + _separator
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

defaults to expected behavior, but this could likely be better since with_port is only actually useful when emulating.

@@ -46,7 +46,7 @@ func _check_emulating() -> void :
if port == "":
Firebase._printerr("You are in 'emulated' mode, but the port for Dynamic Links has not been configured.")
else:
_base_url = "http://127.0.0.1:{port}/{version}/".format({ version = _API_VERSION, port = port })
_base_url = "http://localhost:{port}/{version}/".format({ version = _API_VERSION, port = port })
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed some spots using 127.0.0.1 instead of localhost

Comment on lines +151 to +160
if not auth:
Firebase._print("Unauthenticated request issued...")
Firebase.Auth.login_anonymous()
var result : Array = yield(Firebase.Auth, "auth_request")
if result[0] != 1:
_check_auth_error(result[0], result[1])
Firebase._print("Client connected as Anonymous")

if not Firebase.emulating:
task._headers = Array(task._headers) + [_AUTHORIZATION_HEADER + auth.idtoken]

task._headers = Array(task._headers) + [_AUTHORIZATION_HEADER + auth.idtoken]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With changes to the auth.gd error codes this does not fail like it normally would.

Comment on lines +50 to +61
func _parse_response_body(headers):
var body = response_body.get_string_from_utf8()
if body:
var event_data = get_event_data(body)
if event_data.event != "keep-alive" and event_data.event != continue_internal:
var result = JSON.parse(event_data.data).result
if response_body.size() > 0 and result: # stop here if the value doesn't parse
response_body.resize(0)
emit_signal("new_sse_event", headers, event_data.event, result)
else:
if event_data.event != continue_internal:
response_body.resize(0)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved this to it's own function (with headers just as a passthrough)

Comment on lines 92 to 101
elif Firebase.emulating:
# Emulation does not send the close connection header currently, so we need to manually read the response body
# see issue https://github.com/firebase/firebase-tools/issues/3329 in firebase-tools
# also comment https://github.com/GodotNuts/GodotFirebase/issues/154#issuecomment-831377763 which explains the issue
while httpclient.connection.get_available_bytes():
var data = httpclient.connection.get_partial_data(1)
if data[0] == OK:
response_body.append_array(data[1])
if response_body.size() > 0:
_parse_response_body(headers)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a workaround for emulators not sending the Connection: closed header. This just looks to see if we still have data even if it wasn't detected by Godot and parses it.

@LeonardMeagher2 LeonardMeagher2 marked this pull request as ready for review December 7, 2021 03:58
@LeonardMeagher2 LeonardMeagher2 changed the title Fixing emulation Fixing Database emulation Dec 7, 2021
Comment on lines +70 to +72
"workarounds":{
"database_connection_closed_issue": false, # fixes https://github.com/firebase/firebase-tools/issues/3329
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a flag for the workaround disabling it by default since it only effects emulation
@WolfgangSenff

@BearDooks
Copy link
Member

@WolfgangSenff I ran this through our tools and it seems to be working fine. No errors when running the test harness

@WolfgangSenff WolfgangSenff merged commit 122e99d into GodotNuts:main Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants