Godot NakamaClient.gd issue with "list_matches_async" function about nakama-godot HOT 6 CLOSED

If filter or query aren't needed the server expects no input for those fields, rather than "". The parameter should be missing entirely from the input if possible.

from nakama-godot.

Hey @Faless I'm looking at the .NET client right now to see if we ever encode empty strings.

from nakama-godot.

I can confirm the issue, a quick fix is to change the NakamaClient.list_matches_async to pass null if the string is empty:

diff --git a/addons/com.heroiclabs.nakama/client/NakamaClient.gd b/addons/com.heroiclabs.nakama/client/NakamaClient.gd
index 2f36498..2859949 100644
--- a/addons/com.heroiclabs.nakama/client/NakamaClient.gd
+++ b/addons/com.heroiclabs.nakama/client/NakamaClient.gd
@@ -590,7 +590,7 @@ func list_leaderboard_records_around_owner_async(p_session : NakamaSession,
 # Returns a task which resolves to the match list object.
 func list_matches_async(p_session : NakamaSession, p_min : int, p_max : int, p_limit : int, p_authoritative : bool,
                p_label : String, p_query : String): # -> NakamaAPI.ApiMatchList:
-       return _api_client.list_matches_async(p_session, p_limit, p_authoritative, p_label, p_min, p_max, p_query)
+       return _api_client.list_matches_async(p_session, p_limit, p_authoritative, p_label if p_label else null, p_min, p_max, p_query if p_query else null)
 
 # List notifications for the user with an optional cursor.
 # @param p_session - The session of the user.

I wonder if a better solution would be to have the auto-generated code always skip encoding empty string. But I fell this might break some other endpoints if there is any that specifically wants an empty string encoded as somestring=. @novabyte ideas ?

from nakama-godot.

@Faless We have the same situation in the .NET client where empty strings get tacked on as query parameters. null strings, however, do not. So I think suggesting the passing in of null and not appending the query param key or value if so, works for now.

Unless @zyro happens to know if the server handles a query param with an empty string value equivalently to the absence of the query param. But I'd be surprised if that worked in all cases.

from nakama-godot.

I can confirm the issue, a quick fix is to change the NakamaClient.list_matches_async to pass null if the string is empty:

diff --git a/addons/com.heroiclabs.nakama/client/NakamaClient.gd b/addons/com.heroiclabs.nakama/client/NakamaClient.gd
index 2f36498..2859949 100644
--- a/addons/com.heroiclabs.nakama/client/NakamaClient.gd
+++ b/addons/com.heroiclabs.nakama/client/NakamaClient.gd
@@ -590,7 +590,7 @@ func list_leaderboard_records_around_owner_async(p_session : NakamaSession,
 # Returns a task which resolves to the match list object.
 func list_matches_async(p_session : NakamaSession, p_min : int, p_max : int, p_limit : int, p_authoritative : bool,
                p_label : String, p_query : String): # -> NakamaAPI.ApiMatchList:
-       return _api_client.list_matches_async(p_session, p_limit, p_authoritative, p_label, p_min, p_max, p_query)
+       return _api_client.list_matches_async(p_session, p_limit, p_authoritative, p_label if p_label else null, p_min, p_max, p_query if p_query else null)
 
 # List notifications for the user with an optional cursor.
 # @param p_session - The session of the user.

I wonder if a better solution would be to have the auto-generated code always skip encoding empty string. But I fell this might break some other endpoints if there is any that specifically wants an empty string encoded as somestring=. @novabyte ideas ?

Nice works now, just also dont forget to change the: p_label : String, p_query : String to just p_label,p_query

from nakama-godot.

We have the same situation in the .NET client where empty strings get tacked on as query parameters. null strings, however, do not. So I think suggesting the passing in of null and not appending the query param key or value if so, works for now.

Unfortunately, in GDScript, an argument that has a type hint of "String" can't accept null as a value. The only way to allow users to pass null for these arguments to NakamaClient.list_matches_async() would be to remove the type hint altogether.

I wonder if a better solution would be to have the auto-generated code always skip encoding empty string.

This scares me because it would make it impossible for a user to send any empty string in a query argument to Nakama, because all empty strings would always be skipped. This would also differ from the behavior of the .NET library, where you can pass in empty strings, it's just that string arguments are nullable in C# (at least before C# 8.0 or when the nullable context is disabled). Even if the current Nakama API has no situation where we'd want to pass an empty string as a query argument (I actually don't know if that's true), if something were added later, the Godot client would be at a disadvantage compared to the .NET library, if we made the decision to always omit empty strings.

So, I think @Faless' "quick fix" is actually the best fix. If the choice is between (1) removing the type hint on those arguments entirely, or (2) adding a special case for these two argument on this specific method, I think option nr 2 is the lesser "evil". Option nr 1 would remove type safety which I think would worsen the developer experience.

Here's PR #93 implementing that fix

from nakama-godot.