You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "axreldable (via GitHub)" <gi...@apache.org> on 2026/02/09 20:24:03 UTC
[PR] GH-139: [Flight] Stop return null from MetadataAdapter.getAll(String) and getAllByte(String) [arrow-java]
axreldable opened a new pull request, #1016:
URL: https://github.com/apache/arrow-java/pull/1016
## What's Changed
`CallHeaders` has 3 implementations:
- FlightCallHeaders
- ErrorFlightMetadata
- MetadataAdapter
**Before this change:**
`MetadataAdapter` could return `null` from `getAll(String)` and `getAllByte(String)` when there were no values for the key, because gRPC’s `Metadata.getAll()` returns `null` in that case. This was undocumented and forced callers to null-check.
**After this change:**
All 3 implementations consistently return an `empty iterable` (never `null`) when the key is absent or has no values. The contract is documented on the interface and covered by tests for each implementation.
**This contains breaking changes.**
`MetadataAdapter.getAll(String)` and `getAllByte(String)` return empty iterator instead of null.
Closes #139.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-139: [Flight] Stop return null from MetadataAdapter.getAll(String) and getAllByte(String) [arrow-java]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #1016:
URL: https://github.com/apache/arrow-java/pull/1016#issuecomment-3915755556
@axreldable let me take a look. I will get back to you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-139: [Flight] Stop return null from MetadataAdapter.getAll(String) and getAllByte(String) [arrow-java]
Posted by "axreldable (via GitHub)" <gi...@apache.org>.
axreldable commented on PR #1016:
URL: https://github.com/apache/arrow-java/pull/1016#issuecomment-3915701366
@jbonofre , some CI pipelines failed. Do you think it relates to the changes or the pipelines are flaky?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-139: [Flight] Stop return null from MetadataAdapter.getAll(String) and getAllByte(String) [arrow-java]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #1016:
URL: https://github.com/apache/arrow-java/pull/1016#discussion_r2816990217
##########
flight/flight-core/src/main/java/org/apache/arrow/flight/ServerSessionMiddleware.java:
##########
@@ -80,20 +80,18 @@ public ServerSessionMiddleware onCallStarted(
String sessionId = null;
final Iterable<String> it = incomingHeaders.getAll("cookie");
- if (it != null) {
- findIdCookie:
- for (final String headerValue : it) {
- for (final String cookie : headerValue.split(" ;")) {
- final String[] cookiePair = cookie.split("=");
- if (cookiePair.length != 2) {
- // Soft failure: Ignore invalid cookie list field
- break;
- }
-
- if (sessionCookieName.equals(cookiePair[0]) && cookiePair[1].length() > 0) {
- sessionId = cookiePair[1];
- break findIdCookie;
- }
+ findIdCookie:
Review Comment:
Ah yes, I got it. Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-139: [Flight] Stop return null from MetadataAdapter.getAll(String) and getAllByte(String) [arrow-java]
Posted by "axreldable (via GitHub)" <gi...@apache.org>.
axreldable commented on PR #1016:
URL: https://github.com/apache/arrow-java/pull/1016#issuecomment-3873663863
Please add the `enhancement` label.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-139: [Flight] Stop return null from MetadataAdapter.getAll(String) and getAllByte(String) [arrow-java]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on PR #1016:
URL: https://github.com/apache/arrow-java/pull/1016#issuecomment-3920388222
The verify job failed to flaky test (memory allocation). I ran again the jobs to double check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-139: [Flight] Stop return null from MetadataAdapter.getAll(String) and getAllByte(String) [arrow-java]
Posted by "axreldable (via GitHub)" <gi...@apache.org>.
axreldable commented on PR #1016:
URL: https://github.com/apache/arrow-java/pull/1016#issuecomment-3876405199
> Hello @axreldable This looks good to me, thank you for it.
>
> I note that this also fixes an NPE in `MetadataAdapter.getAllByte(String key)`
Correct, it fixes NPE for `getAll(key).spliterator()` in `MetadataAdapter.getAllByte(String key)` method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-139: [Flight] Stop return null from MetadataAdapter.getAll(String) and getAllByte(String) [arrow-java]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre commented on code in PR #1016:
URL: https://github.com/apache/arrow-java/pull/1016#discussion_r2816933193
##########
flight/flight-core/src/main/java/org/apache/arrow/flight/client/ClientCookieMiddleware.java:
##########
@@ -100,10 +100,7 @@ public void onBeforeSendingHeaders(CallHeaders outgoingHeaders) {
@Override
public void onHeadersReceived(CallHeaders incomingHeaders) {
- final Iterable<String> setCookieHeaders = incomingHeaders.getAll(SET_COOKIE_HEADER);
- if (setCookieHeaders != null) {
- factory.updateCookies(setCookieHeaders);
- }
+ factory.updateCookies(incomingHeaders.getAll(SET_COOKIE_HEADER));
Review Comment:
nit: if I agree with this change, it's not strictly related to the PR (just to keep the review focused).
##########
flight/flight-core/src/main/java/org/apache/arrow/flight/ServerSessionMiddleware.java:
##########
@@ -80,20 +80,18 @@ public ServerSessionMiddleware onCallStarted(
String sessionId = null;
final Iterable<String> it = incomingHeaders.getAll("cookie");
- if (it != null) {
- findIdCookie:
- for (final String headerValue : it) {
- for (final String cookie : headerValue.split(" ;")) {
- final String[] cookiePair = cookie.split("=");
- if (cookiePair.length != 2) {
- // Soft failure: Ignore invalid cookie list field
- break;
- }
-
- if (sessionCookieName.equals(cookiePair[0]) && cookiePair[1].length() > 0) {
- sessionId = cookiePair[1];
- break findIdCookie;
- }
+ findIdCookie:
Review Comment:
Should not we deal with the case where `it` is `null` ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-139: [Flight] Stop return null from MetadataAdapter.getAll(String) and getAllByte(String) [arrow-java]
Posted by "axreldable (via GitHub)" <gi...@apache.org>.
axreldable commented on code in PR #1016:
URL: https://github.com/apache/arrow-java/pull/1016#discussion_r2816982908
##########
flight/flight-core/src/main/java/org/apache/arrow/flight/client/ClientCookieMiddleware.java:
##########
@@ -100,10 +100,7 @@ public void onBeforeSendingHeaders(CallHeaders outgoingHeaders) {
@Override
public void onHeadersReceived(CallHeaders incomingHeaders) {
- final Iterable<String> setCookieHeaders = incomingHeaders.getAll(SET_COOKIE_HEADER);
- if (setCookieHeaders != null) {
- factory.updateCookies(setCookieHeaders);
- }
+ factory.updateCookies(incomingHeaders.getAll(SET_COOKIE_HEADER));
Review Comment:
I believe it relates to the PR as `CallHeaders.getAll(String key)` doesn't return `null`, so we don't have to check it here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-139: [Flight] Stop return null from MetadataAdapter.getAll(String) and getAllByte(String) [arrow-java]
Posted by "jbonofre (via GitHub)" <gi...@apache.org>.
jbonofre merged PR #1016:
URL: https://github.com/apache/arrow-java/pull/1016
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-139: [Flight] Stop return null from MetadataAdapter.getAll(String) and getAllByte(String) [arrow-java]
Posted by "axreldable (via GitHub)" <gi...@apache.org>.
axreldable commented on PR #1016:
URL: https://github.com/apache/arrow-java/pull/1016#issuecomment-3921144344
Thank you, @jbonofre ! Looks like tests passed this time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-139: [Flight] Stop return null from MetadataAdapter.getAll(String) and getAllByte(String) [arrow-java]
Posted by "axreldable (via GitHub)" <gi...@apache.org>.
axreldable commented on code in PR #1016:
URL: https://github.com/apache/arrow-java/pull/1016#discussion_r2816977508
##########
flight/flight-core/src/main/java/org/apache/arrow/flight/ServerSessionMiddleware.java:
##########
@@ -80,20 +80,18 @@ public ServerSessionMiddleware onCallStarted(
String sessionId = null;
final Iterable<String> it = incomingHeaders.getAll("cookie");
- if (it != null) {
- findIdCookie:
- for (final String headerValue : it) {
- for (final String cookie : headerValue.split(" ;")) {
- final String[] cookiePair = cookie.split("=");
- if (cookiePair.length != 2) {
- // Soft failure: Ignore invalid cookie list field
- break;
- }
-
- if (sessionCookieName.equals(cookiePair[0]) && cookiePair[1].length() > 0) {
- sessionId = cookiePair[1];
- break findIdCookie;
- }
+ findIdCookie:
Review Comment:
After this change all 3 implementations of `CallHeaders.getAll(String key)` return an `empty iterable` (never null). So, `it` is not `null` here.
The new doc string for `CallHeaders.getAll(String key)`
```
/**
* Get all values present for the given metadata key.
*
* @param key the metadata key
* @return an iterable of all values for the key. Returns an empty iterable if no value to return.
*/
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-139: [Flight] Stop return null from MetadataAdapter.getAll(String) and getAllByte(String) [arrow-java]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #1016:
URL: https://github.com/apache/arrow-java/pull/1016#issuecomment-3873657554
<!-- label_helper_comment -->
Thank you for opening a pull request!
Please label the PR with one or more of:
- bug-fix
- chore
- dependencies
- documentation
- enhancement
Also, add the 'breaking-change' label if appropriate.
See [CONTRIBUTING.md](https://github.com/apache/arrow-java/blob/main/CONTRIBUTING.md) for details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org