DeepLink Query Parameter Value Template Matching

[yutailang0119]

This PR addresses an issue where DeepLink handling fails when the query parameter's key and its value template are different, leading to an ArrayIndexOutOfBoundsException.

Issue

When using a uriPattern in Navigation 2 that includes a query parameter with a value template (e.g., ?firstName={first_name}), the nav3-recipes implementation currently fails if the key (firstName) does not exactly match the template variable (first_name).

This happens because the current logic incorrectly assumes that the query parameter key will always be identical to the variable name (the value template) and does not account for cases where the two might differ, such as when using @SerialName.

Reproduction

The issue is triggered if a key uses a different serialization name than its property name, for example:

@Serializable
internal data class SearchKey(
    @SerialName("first_name") // Key is "first_name"
    val firstName: String? = null, // Value template is "{firstName}"
    // ...
)

In the DeepLink URI pattern: ...?first_name={firstName}

This recipe currently throws an ArrayIndexOutOfBoundsException when trying to process this mismatched key/value template pair.

Solution

This fix modifies the DeepLink processing to correctly handle and match the parameter key with the separate parameter value template, resolving the ArrayIndexOutOfBoundsException and allowing for flexibility in parameter naming (e.g., when using @SerialName).

[gemini-code-assist]

Summary of Changes

Hello @yutailang0119, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses a critical issue in the nav3-recipes DeepLink implementation where an ArrayIndexOutOfBoundsException would occur if a DeepLink's query parameter key differed from its value template, often seen with @SerialName usage. The changes enhance the DeepLink matcher to accurately interpret and map these parameters, ensuring robust handling of diverse naming conventions and preventing runtime errors, thereby improving the overall stability and flexibility of DeepLink processing.

Highlights

• DeepLink Handling Fix: Resolves an ArrayIndexOutOfBoundsException that occurred when DeepLink query parameter keys did not exactly match their corresponding value templates, particularly when using @SerialName.
• Flexible Query Parameter Matching: Modifies the DeepLink processing logic to correctly handle and match query parameter keys with separate parameter value templates, allowing for greater flexibility in parameter naming.
• Introduction of QueryParameter Class: Introduces a new QueryParameter class to store metadata about supported query parameters, including the serialName (the actual argument name from the template) and its typeParser.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request correctly addresses the issue of mismatched query parameter keys and value templates in deep links. The changes in DeepLinkPattern.kt are well-implemented to extract both the query key and the property name from the URI pattern.

My review includes two main points:

1. A critical fix in DeepLinkMatcher.kt to prevent a potential NullPointerException when handling unknown query parameters in a requested URI.
2. A suggestion in DeepLinkPattern.kt to convert the new QueryParameter class to a data class for better maintainability, following Kotlin best practices.

Overall, the changes are good, but the potential crash needs to be addressed.

[gemini-code-assist]

The use of the non-null assertion operator (!!) on line 51 can cause a NullPointerException if the request URI contains a query parameter that is not defined in the deep link pattern. This would crash the application. It's safer to handle this case by treating it as a non-match, which is consistent with how path segments are handled. Using a for loop instead of forEach allows for an early return from the match function if an unknown parameter is found.

Suggested change

	request.queries.forEach { query ->
	val name = query.key
	val queryStringParser = deepLinkPattern.queryValueParsers[name]
	val queryParsedValue = try {
	queryStringParser!!.invoke(query.value)
	val queryParameter = deepLinkPattern.queryParameters[name]!!
	val (argName, parsedValue) = try {
	Pair(queryParameter.serialName ?: name, queryParameter.typeParser.invoke(query.value))
	} catch (e: IllegalArgumentException) {
	Log.e(TAG_LOG_ERROR, "Failed to parse query name:[$name] value:[${query.value}].", e)
	return null
	}
	args[name] = queryParsedValue
	args[argName] = parsedValue
	}
	for (query in request.queries) {
	val name = query.key
	val queryParameter = deepLinkPattern.queryParameters[name] ?: return null
	val (argName, parsedValue) = try {
	Pair(queryParameter.serialName ?: name, queryParameter.typeParser.invoke(query.value))
	} catch (e: IllegalArgumentException) {
	Log.e(TAG_LOG_ERROR, "Failed to parse query name:[$name] value:[${query.value}].", e)
	return null
	}
	args[argName] = parsedValue
	}

[gemini-code-assist]

For classes that primarily hold data, like QueryParameter, it's a good practice to declare them as a data class. This automatically generates useful methods like equals(), hashCode(), toString(), and copy(), which improves code conciseness and can be helpful for debugging.

Suggested change

	class QueryParameter(
	val serialName: String?,
	val typeParser: TypeParser,
	)
	data class QueryParameter(
	val serialName: String?,
	val typeParser: TypeParser,
	)

[claraf3]

Hi thanks for the PR!

The query matching in this recipe is based on matching the deeplink url's query parameter name to the Serializer's elementName.

The original recipe's query paramName is based on SearchKey::firstName.name, and the ElementName is firstName so it matches.

In your solution, you updated the ElementName to "first_name" via SerialName, but the url's query paramName remains as SearchKey::firstName.name ("firstName"), so it doesn't work. (The solution throws if you added @SerialName("first_name")).

So the key point here is making sure the url's paramName matches the serialName, and not necessarily whether the serialName matches the template name.

So if you do want to support @SerialName("first_name"), all you should be doing is making sure your Url paramName matches SerialName, i.e. building url via the class's Encoder. The rest of the code can stay the same.

[yutailang0119]

Thank you for the response!

I agree that the difficulty arises because the sample use case currently constructs the URL using SearchKey::firstName.name, which prevents the incorporation of a path parameter like {first_name}.

To allow the use of @SerialName("first_name"), it seems necessary to remove the restriction that the URL is assembled solely from the elementName.

However, since the query matching is based on Serializable, the use of @SerialName is still possible within that context.

Given this, would the approach of restricting the use of @SerialName for variables in URL?
e.g., by throwing an appropriate exception within deepLinkPattern.queryValueParsers[name] when an unsupported name (like one derived from @SerialName) is encountered.