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.

Sign up for GitHub

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

Auth Manager API part 1: HTTPRequest, HTTPHeader #11769

Merged
merged 8 commits into from
Dec 17, 2024
+560 −0
Merged

Auth Manager API part 1: HTTPRequest, HTTPHeader #11769

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7db53d8
Auth Manager API part 1: HTTPRequest, HTTPHeader
adutra Dec 12, 2024
e486e0d
review
adutra Dec 13, 2024
0449744
remove static methods
adutra Dec 13, 2024
d9a05fb
verify error messages
adutra Dec 13, 2024
4c0e650
checkstyle
adutra Dec 13, 2024
56ff7b2
review
adutra Dec 13, 2024
ca6eb1e
review
adutra Dec 16, 2024
2848e42
review
adutra Dec 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
remove static methods
  • Loading branch information
@adutra
adutra committed Dec 13, 2024
commit 0449744f7227af8a7507cdffdc71e24e3172518f
69 changes: 0 additions & 69 deletions core/src/main/java/org/apache/iceberg/rest/HTTPHeaders.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,7 @@
package org.apache.iceberg.rest;

import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.stream.Collectors;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableListMultimap;
import org.apache.iceberg.relocated.com.google.common.collect.ListMultimap;
import org.apache.iceberg.relocated.com.google.common.collect.Multimap;
import org.immutables.value.Value;

/**
Expand All @@ -46,53 +41,6 @@ public interface HTTPHeaders {
/** Returns all the header entries in this group. */
List<HTTPHeader> entries();
Copy link
Contributor

@danielcweeks danielcweeks Dec 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't this be Set<HTTPHeader>? This would also take care of cases where there are actualy k/v duplicates (which we don't appear to handle)

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we should also annotate this as @NotNull

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 don't see a @NotNull annotation available in iceberg-core? Besides, the immutable generated class is annotated with @AllParametersAreNonNullByDefault.

Copy link
Contributor

Choose a reason for hiding this comment

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

@danielcweeks @NotNull shouldn't be needed here, because everything not marked as @Nullable or Optional is implicitly not null with Immutables


/**
* Returns a map representation of the headers where each header name is mapped to a list of its
* values. Header names are case-insensitive.
*/
@Value.Lazy
default Map<String, List<String>> asMap() {
return entries().stream()
.collect(Collectors.groupingBy(h -> h.name().toLowerCase(Locale.ROOT)))
.values()
.stream()
.collect(
Collectors.toMap(
headers -> headers.get(0).name(),
headers -> headers.stream().map(HTTPHeader::value).collect(Collectors.toList())));
}

/**
* Returns a simple map representation of the headers where each header name is mapped to its
* first value. If a header has multiple values, only the first value is used. Header names are
* case-insensitive.
*/
@Value.Lazy
default Map<String, String> asSimpleMap() {
return entries().stream()
.collect(Collectors.groupingBy(h -> h.name().toLowerCase(Locale.ROOT)))
.values()
.stream()
.collect(
Collectors.toMap(headers -> headers.get(0).name(), headers -> headers.get(0).value()));
}

/**
* Returns a {@link ListMultimap} representation of the headers. Header names are
* case-insensitive.
*/
@Value.Lazy
default ListMultimap<String, String> asMultiMap() {
return entries().stream()
.collect(Collectors.groupingBy(h -> h.name().toLowerCase(Locale.ROOT)))
.values()
.stream()
.collect(
ImmutableListMultimap.flatteningToImmutableListMultimap(
headers -> headers.get(0).name(),
headers -> headers.stream().map(HTTPHeader::value)));
}

/** Returns all the entries in this group for the given name (case-insensitive). */
default List<HTTPHeader> entries(String name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, set

return entries().stream()
Expand Down Expand Up @@ -133,23 +81,6 @@ static HTTPHeaders of(HTTPHeader... headers) {
return ImmutableHTTPHeaders.builder().addEntries(headers).build();
}

static HTTPHeaders fromMap(Map<String, ? extends Iterable<String>> headers) {
ImmutableHTTPHeaders.Builder builder = ImmutableHTTPHeaders.builder();
headers.forEach(
(name, values) -> values.forEach(value -> builder.addEntry(HTTPHeader.of(name, value))));
return builder.build();
}

static HTTPHeaders fromSimpleMap(Map<String, String> headers) {
ImmutableHTTPHeaders.Builder builder = ImmutableHTTPHeaders.builder();
headers.forEach((name, value) -> builder.addEntry(HTTPHeader.of(name, value)));
return builder.build();
}

static HTTPHeaders fromMultiMap(Multimap<String, String> headers) {
return fromMap(headers.asMap());
}

/** Represents an HTTP header as a name-value pair. */
@Value.Style(redactedMask = "****", depluralize = true)
@Value.Immutable
Expand Down
137 changes: 6 additions & 131 deletions core/src/test/java/org/apache/iceberg/rest/TestHTTPHeaders.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,6 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;

import java.util.List;
import java.util.Map;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableListMultimap;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.relocated.com.google.common.collect.Maps;
import org.apache.iceberg.rest.HTTPHeaders.HTTPHeader;
import org.junit.jupiter.api.Test;

Expand All @@ -38,32 +32,6 @@ class TestHTTPHeaders {
HTTPHeader.of("HEADER1", "value1b"),
HTTPHeader.of("header2", "value2"));

@Test
void asMap() {
assertThat(headers.asMap())
.isEqualTo(
Map.of(
"header1", List.of("value1a", "value1b"),
"header2", List.of("value2")));
}

@Test
void asSimpleMap() {
assertThat(headers.asSimpleMap())
.isEqualTo(
Map.of(
"header1", "value1a",
"header2", "value2"));
}

@Test
void asMultiMap() {
assertThat(headers.asMultiMap())
.isEqualTo(
ImmutableListMultimap.of(
"header1", "value1a", "header1", "value1b", "header2", "value2"));
}

@Test
void entries() {
assertThat(headers.entries("header1"))
danielcweeks marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -94,12 +62,12 @@ void addIfAbsentHTTPHeader() {
assertThat(actual).isSameAs(headers);

actual = headers.addIfAbsent(HTTPHeader.of("header3", "value3"));
assertThat(actual.asMap())
.isEqualTo(
Map.of(
"header1", List.of("value1a", "value1b"),
"header2", List.of("value2"),
"header3", List.of("value3")));
assertThat(actual.entries())
.containsExactly(
HTTPHeader.of("header1", "value1a"),
HTTPHeader.of("HEADER1", "value1b"),
HTTPHeader.of("header2", "value2"),
HTTPHeader.of("header3", "value3"));

assertThatThrownBy(() -> headers.addIfAbsent((HTTPHeaders) null))
.isInstanceOf(NullPointerException.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

checks like this should always have a .hasMessage check to make sure we get the right error msg back (unfortunately we can't easily enforce such a rule via checkstyle)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The message is generated by Immutables and will be rather cryptic. But OK.

Expand Down Expand Up @@ -130,99 +98,6 @@ void addIfAbsentHTTPHeaders() {
.isInstanceOf(NullPointerException.class);
}

@Test
void fromMap() {
HTTPHeaders actual =
HTTPHeaders.fromMap(
ImmutableMap.of(
"header1", List.of("value1a", "value1b"),
"header2", List.of("value2")));
assertThat(actual)
.isEqualTo(
ImmutableHTTPHeaders.builder()
.addEntry(HTTPHeader.of("header1", "value1a"))
.addEntry(HTTPHeader.of("header1", "value1b"))
.addEntry(HTTPHeader.of("header2", "value2"))
.build());

// invalid input (null name or value)
assertThatThrownBy(
() -> {
Map<String, List<String>> map = Maps.newHashMap();
map.put(null, Lists.newArrayList("value1"));
HTTPHeaders.fromMap(map);
})
.isInstanceOf(NullPointerException.class);
assertThatThrownBy(
() -> {
Map<String, List<String>> map = Maps.newHashMap();
map.put("header", null);
HTTPHeaders.fromMap(map);
})
.isInstanceOf(NullPointerException.class);
assertThatThrownBy(
() -> HTTPHeaders.fromMap(Map.of("header1", Lists.newArrayList("value1", null))))
.isInstanceOf(NullPointerException.class);

// invalid input (empty name)
assertThatThrownBy(() -> HTTPHeaders.fromMap(Map.of("", List.of("value1"))))
.isInstanceOf(IllegalArgumentException.class);
}

@Test
void fromSimpleMap() {
HTTPHeaders actual =
HTTPHeaders.fromSimpleMap(
ImmutableMap.of(
"header1", "value1a",
"header2", "value2"));
assertThat(actual)
.isEqualTo(
ImmutableHTTPHeaders.builder()
.addEntry(HTTPHeader.of("header1", "value1a"))
.addEntry(HTTPHeader.of("header2", "value2"))
.build());

// invalid input (null name or value)
assertThatThrownBy(
() -> {
Map<String, String> map = Maps.newHashMap();
map.put(null, "value1");
HTTPHeaders.fromSimpleMap(map);
})
.isInstanceOf(NullPointerException.class);
assertThatThrownBy(
() -> {
Map<String, String> map = Maps.newHashMap();
map.put("header", null);
HTTPHeaders.fromSimpleMap(map);
})
.isInstanceOf(NullPointerException.class);

// invalid input (empty name)
assertThatThrownBy(() -> HTTPHeaders.fromSimpleMap(Map.of("", "value1")))
.isInstanceOf(IllegalArgumentException.class);
}

@Test
void fromMultiMap() {
HTTPHeaders actual =
HTTPHeaders.fromMultiMap(
ImmutableListMultimap.of(
"header1", "value1a", "header2", "value2", "header1", "value1b"));
assertThat(actual)
.isEqualTo(
ImmutableHTTPHeaders.builder()
.addEntry(HTTPHeader.of("header1", "value1a"))
.addEntry(HTTPHeader.of("header1", "value1b"))
.addEntry(HTTPHeader.of("header2", "value2"))
.build());

// invalid input (empty name)
assertThatThrownBy(() -> HTTPHeaders.fromMultiMap(ImmutableListMultimap.of("", "value1")))
.isInstanceOf(IllegalArgumentException.class);
}

@Test
void invalidHeader() {
// invalid input (null name or value)
Expand Down
Loading