simplify/robustify WebSocketHttpHeaders.java getSecWebSocketProtocol() ?

Open Beginner friendly
#37,282 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
2/5
Estimated time
1-3 hours
Newbie friendliness
66/100
Issue type
Bug
Clarity
Mostly clear
Activity status
Active
Tech stack
java, spring
Domain
api, backend

Research direction

Start in spring-websocket/src/main/java/org/springframework/web/socket/WebSocketHttpHeaders.java at getSecWebSocketProtocol(), then compare it with getValuesAsList() and the analogous method in spring-web/src/main/java/org/springframework/http/HttpHeaders.java. Reproduce the single-header and repeated-header examples, clarify the intended quote handling, and verify that the final behavior consistently parses the supported WebSocket protocol header forms.

Written by the indexing model from the issue text.

Description

status: waiting-for-triage

Hi, while browsing the code, I was suprised by the logic in https://github.com/spring-projects/spring-framework/blob/main/spring-websocket/src/main/java/org/springframework/web/socket/WebSocketHttpHeaders.java#L162

	public List<String> getSecWebSocketProtocol() {
		List<String> values = get(SEC_WEBSOCKET_PROTOCOL);
		if (CollectionUtils.isEmpty(values)) {
			return Collections.emptyList();
		}
		else if (values.size() == 1) {
			return getValuesAsList(SEC_WEBSOCKET_PROTOCOL);
		}
		else {
			return values;
		}
	}

it would make the code "fail" when request comes with
protocol: foo
protocol: bar, baz

return ["foo", "bar, baz"] instead of ["foo", "bar", "baz"]

Also unify the quote handling (even though the rfc explicitly forbids quotes for sec-websocket-protocol, but at least this would be uniform. but even better would maybe be to reject or just ignore quotes ?) :
protocol: "foo", "bar" returns ["foo", "bar"]
vs
protocol: "foo"
protocol: "bar"
returns [""foo"", ""bar""]

It looks like it's the only place in the whole codebase with this extra check for size() == 1

$ git grep -C 3 "size() == 1" | grep -i AsList
spring-websocket/src/main/java/org/springframework/web/socket/WebSocketHttpHeaders.java-			return getValuesAsList(SEC_WEBSOCKET_PROTOCOL);

seems like it was iterated on in https://github.com/spring-projects/spring-framework/commit/55dae618a64da520d9154fe17f1529acab45873a#diff-e2d6218e6585f8b7e32682f6f8f7aed22dbd76d862ec92e86e0902243889556aL462-R471 and

It's obviously an edge case, but unless I missed somthing, it becomes simpler/more robust to just change it to

	public List<String> getSecWebSocketProtocol() {
			return getValuesAsList(SEC_WEBSOCKET_PROTOCOL);
	}

like in all methods in the parent class HttpHeaders.java (e.g. https://github.com/spring-projects/spring-framework/blob/main/spring-web/src/main/java/org/springframework/http/HttpHeaders.java#L706

	public List<String> getAccessControlAllowHeaders() {
		return getValuesAsList(ACCESS_CONTROL_ALLOW_HEADERS);
	}

I just saw this in passing, just though it'd mention it (obviously I'm leaving all the hard work of knowing what to do to you...). Feel free to close if appropriate

Dominant language
Java
Stars
60.2k
Forks
38.8k
Avg merge
4d 11h
Merged PRs (30d)
30

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from spring-projects/spring-framework

All issues in spring-projects/spring-framework

Similar issues

More Java issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.