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.

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

Add convenience Content.withId(String) #6937

Merged
merged 2 commits into from
May 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
import org.projectnessie.model.Content;
import org.projectnessie.model.ContentKey;
import org.projectnessie.model.GetMultipleContentsResponse;
import org.projectnessie.model.ImmutableNamespace;
import org.projectnessie.model.Namespace;
import org.projectnessie.model.Operation.Put;

Expand Down Expand Up @@ -61,7 +60,7 @@ public CreateNamespaceResult createWithResponse()
throw new IllegalArgumentException("Creating empty namespaces is not supported");
}

ImmutableNamespace content = Namespace.builder().from(namespace).properties(properties).build();
Namespace content = Namespace.builder().from(namespace).properties(properties).build();
ContentKey key = namespace.toContentKey();

GetMultipleContentsResponse contentsResponse;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,30 @@ default Map<ContentKey, String> toAddedContentsMap() {
return added.stream().collect(Collectors.toMap(AddedContent::getKey, AddedContent::contentId));
}

/**
* If new content has been added to Nessie for the given {@link ContentKey key}, updates the
* {@link Content#getId() content ID} of the given {@link Content content} with the content ID
* returned by Nessie.
*
* <p>Returns the {@code content} parameter value, if no content for the given {@code key} has
* been added.
*
* <p>Note: This convenience function only works for REST API v2.
*/
@SuppressWarnings("unchecked")
default <T extends Content> T contentWithAssignedId(ContentKey key, T content) {
List<AddedContent> addedContents = getAddedContents();
if (addedContents == null) {
throw new UnsupportedOperationException("toAddedContentsMap is not available in API v1");
}
return (T)
getAddedContents().stream()
.filter(added -> added.getKey().equals(key))
.findFirst()
.map(added -> content.withId(added.contentId()))
dimas-b marked this conversation as resolved.
Show resolved Hide resolved
.orElse(content);
}

@Value.Immutable
@JsonSerialize(as = ImmutableAddedContent.class)
@JsonDeserialize(as = ImmutableAddedContent.class)
Expand Down
2 changes: 2 additions & 0 deletions api/model/src/main/java/org/projectnessie/model/Content.java
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,8 @@ public interface Type {
@JsonIgnore
public abstract Type getType();

public abstract Content withId(String id);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe:

Suggested change
public abstract Content withId(String id);
public abstract Content withId(@Nullable @jakarta.annotation.Nullable String id);

Conversely we could require id to be non null and introduce a withoutId() method, wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope - ID must be nullable - otherwise you could never add new content


/**
* Unwrap object if possible, otherwise throw.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,4 +45,7 @@ public abstract class DeltaLakeTable extends Content {
public Type getType() {
return Type.DELTA_LAKE_TABLE;
}

@Override
public abstract DeltaLakeTable withId(String id);
}
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,9 @@ public Type getType() {
// To be removed when API v1 gets removed.
public abstract Map<String, Object> getMetadata();

@Override
public abstract IcebergTable withId(String id);

public static ImmutableIcebergTable.Builder builder() {
return ImmutableIcebergTable.builder();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@ public Type getType() {
// To be removed when API v1 gets removed.
public abstract Map<String, Object> getMetadata();

@Override
public abstract IcebergView withId(String id);

public static ImmutableIcebergView.Builder builder() {
return ImmutableIcebergView.builder();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,9 @@ public Namespace getParent() {
@JsonInclude(Include.NON_EMPTY)
public abstract Map<String, String> getProperties();

@Override
public abstract Namespace withId(String id);

/**
* Builds a {@link Namespace} using the elements of the given content key.
*
Expand Down
3 changes: 3 additions & 0 deletions api/model/src/main/java/org/projectnessie/model/UDF.java
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ public Type getType() {
return Type.UDF;
}

@Override
public abstract UDF withId(String id);

public static ImmutableUDF.Builder builder() {
return ImmutableUDF.builder();
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/*
* Copyright (C) 2023 Dremio
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.projectnessie.model;

import static org.projectnessie.model.CommitResponse.AddedContent.addedContent;

import org.assertj.core.api.SoftAssertions;
import org.assertj.core.api.junit.jupiter.InjectSoftAssertions;
import org.assertj.core.api.junit.jupiter.SoftAssertionsExtension;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;

@ExtendWith(SoftAssertionsExtension.class)
public class TestCommitResponse {
@InjectSoftAssertions protected SoftAssertions soft;

@Test
void addedContentKey() {
Branch branch = Branch.of("branch", "11223344");
IcebergTable table = IcebergTable.of("x", 1, 2, 3, 4);

// REST API v1
soft.assertThatThrownBy(
() ->
CommitResponse.builder()
.targetBranch(branch)
.build()
.contentWithAssignedId(ContentKey.of("foo"), table))
.isInstanceOf(UnsupportedOperationException.class);

// REST API v2 cases
soft.assertThat(
CommitResponse.builder()
.addAddedContents(addedContent(ContentKey.of("bar"), "1234"))
.targetBranch(branch)
.build()
.contentWithAssignedId(ContentKey.of("foo"), table))
.isSameAs(table);
soft.assertThat(
CommitResponse.builder()
.addAddedContents(addedContent(ContentKey.of("foo"), "1234"))
.targetBranch(branch)
.build()
.contentWithAssignedId(ContentKey.of("foo"), table))
.isEqualTo(table.withId("1234"));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -265,5 +265,10 @@ public CommitMeta getCommitMeta() {
.commitTime(Instant.ofEpochMilli(1234567890L))
.build();
}

@Override
public MyCustomContent withId(String id) {
throw new UnsupportedOperationException();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@
import org.projectnessie.versioned.storage.testextension.NessieBackend;
import org.projectnessie.versioned.storage.testextension.NessiePersist;
import org.projectnessie.versioned.storage.testextension.PersistExtension;
import org.projectnessie.versioned.store.DefaultStoreWorker;

@ExtendWith({PersistExtension.class, SoftAssertionsExtension.class})
@NessieBackend(InmemoryBackendTestFactory.class)
Expand Down Expand Up @@ -125,9 +124,7 @@ public void commitLog() throws Exception {
op -> {
if (op instanceof Operation.Put) {
return Operation.Put.of(
op.getKey(),
DefaultStoreWorker.instance()
.applyId(((Operation.Put) op).getContent(), null));
op.getKey(), ((Operation.Put) op).getContent().withId(null));
}
return op;
})
Expand Down Expand Up @@ -177,10 +174,7 @@ public void allContents() throws Exception {
nessie.allContents(
Detached.of(current.getKey()), singleton(Content.Type.ICEBERG_TABLE))) {
soft.assertThat(contents)
.map(
e ->
Maps.immutableEntry(
e.getKey(), DefaultStoreWorker.instance().applyId(e.getValue(), null)))
.map(e -> Maps.immutableEntry(e.getKey(), e.getValue().withId(null)))
.containsExactlyInAnyOrderElementsOf(expectedContents.entrySet());
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,11 @@ protected CommitMultipleOperationsBuilder prepCommit(
}

protected Put dummyPut(String... elements) {
return Put.of(ContentKey.of(elements), IcebergTable.of("foo", 1, 2, 3, 4));
return Put.of(ContentKey.of(elements), dummyTable());
}

private static IcebergTable dummyTable() {
return IcebergTable.of("foo", 1, 2, 3, 4);
}

protected boolean fullPagingSupport() {
Expand Down Expand Up @@ -466,6 +470,15 @@ public void commitMergeTransplant() throws Exception {
.hasSize(2)
.extracting(AddedContent::getKey)
.containsExactlyInAnyOrder(ContentKey.of("b", "a"), ContentKey.of("b", "b"));
soft.assertThat(resp.contentWithAssignedId(ContentKey.of("b", "a"), dummyTable()))
.extracting(IcebergTable::getId)
.isNotNull();
soft.assertThat(resp.contentWithAssignedId(ContentKey.of("b", "b"), dummyTable()))
.extracting(IcebergTable::getId)
.isNotNull();
soft.assertThat(resp.contentWithAssignedId(ContentKey.of("x", "y"), dummyTable()))
.extracting(IcebergTable::getId)
.isNull();
} else {
branch = prepCommit(branch, "one", dummyPut("a", "a")).commit();
branch = prepCommit(branch, "two", dummyPut("b", "a"), dummyPut("b", "b")).commit();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -376,19 +376,7 @@ private Operation clearIdOnOperation(Operation o) {
}

private Content clearIdOnContent(Content content) {
return setIdOnContent(content, null);
}

private Content setIdOnContent(Content content, String contentId) {
try {
return (Content)
content
.getClass()
.getDeclaredMethod("withId", String.class)
.invoke(content, new Object[] {contentId});
} catch (Exception e) {
throw new RuntimeException(e);
}
return content.withId(null);
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,15 +50,12 @@
import org.projectnessie.model.CommitResponse;
import org.projectnessie.model.Content;
import org.projectnessie.model.ContentKey;
import org.projectnessie.model.DeltaLakeTable;
import org.projectnessie.model.Detached;
import org.projectnessie.model.DiffResponse.DiffEntry;
import org.projectnessie.model.EntriesResponse.Entry;
import org.projectnessie.model.FetchOption;
import org.projectnessie.model.GetMultipleContentsResponse.ContentWithKey;
import org.projectnessie.model.IcebergTable;
import org.projectnessie.model.IcebergView;
import org.projectnessie.model.ImmutableDeltaLakeTable;
import org.projectnessie.model.ImmutableOperations;
import org.projectnessie.model.LogResponse.LogEntry;
import org.projectnessie.model.Namespace;
Expand All @@ -67,7 +64,6 @@
import org.projectnessie.model.Operations;
import org.projectnessie.model.Reference;
import org.projectnessie.model.Tag;
import org.projectnessie.model.UDF;
import org.projectnessie.services.authz.AbstractBatchAccessChecker;
import org.projectnessie.services.authz.AccessContext;
import org.projectnessie.services.authz.Authorizer;
Expand Down Expand Up @@ -575,37 +571,7 @@ protected static Content contentWithoutId(Content content) {
if (content == null) {
return null;
}
if (content instanceof IcebergTable) {
IcebergTable t = (IcebergTable) content;
return IcebergTable.of(
t.getMetadataLocation(),
t.getSnapshotId(),
t.getSchemaId(),
t.getSpecId(),
t.getSortOrderId());
}
if (content instanceof IcebergView) {
IcebergView t = (IcebergView) content;
return IcebergView.of(
t.getMetadataLocation(),
t.getVersionId(),
t.getSchemaId(),
t.getDialect(),
t.getSqlText());
}
if (content instanceof UDF) {
UDF t = (UDF) content;
return UDF.of(t.getDialect(), t.getSqlText());
}
if (content instanceof DeltaLakeTable) {
DeltaLakeTable t = (DeltaLakeTable) content;
return ImmutableDeltaLakeTable.builder().from(t).id(null).build();
}
if (content instanceof Namespace) {
Namespace t = (Namespace) content;
return Namespace.of(t.getElements());
}
throw new IllegalArgumentException(content.toString());
return content.withId(null);
}

protected static DiffEntry diffEntryWithoutContentId(DiffEntry diff) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import java.util.function.Supplier;
import org.projectnessie.model.Content;
import org.projectnessie.model.DeltaLakeTable;
import org.projectnessie.model.ImmutableDeltaLakeTable;
import org.projectnessie.nessie.relocated.protobuf.ByteString;
import org.projectnessie.server.store.proto.ObjectTypes;

Expand Down Expand Up @@ -47,11 +46,6 @@ protected void toStoreOnRefState(DeltaLakeTable content, ObjectTypes.Content.Bui
builder.setDeltaLakeTable(table);
}

@Override
public DeltaLakeTable applyId(DeltaLakeTable content, String id) {
return ((ImmutableDeltaLakeTable) content).withId(id);
}

@Override
protected DeltaLakeTable valueFromStore(
ObjectTypes.Content content, Supplier<ByteString> globalState) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import java.util.function.Supplier;
import org.projectnessie.model.Content;
import org.projectnessie.model.IcebergTable;
import org.projectnessie.model.ImmutableIcebergTable;
import org.projectnessie.nessie.relocated.protobuf.ByteString;
import org.projectnessie.server.store.proto.ObjectTypes;

Expand Down Expand Up @@ -47,11 +46,6 @@ protected void toStoreOnRefState(IcebergTable table, ObjectTypes.Content.Builder
builder.setIcebergRefState(stateBuilder);
}

@Override
public IcebergTable applyId(IcebergTable content, String id) {
return ((ImmutableIcebergTable) content).withId(id);
}

@Override
public boolean requiresGlobalState(ByteString content) {
ObjectTypes.Content parsed = parse(content);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
import java.util.function.Supplier;
import org.projectnessie.model.Content;
import org.projectnessie.model.IcebergView;
import org.projectnessie.model.ImmutableIcebergView;
import org.projectnessie.nessie.relocated.protobuf.ByteString;
import org.projectnessie.server.store.proto.ObjectTypes;

Expand Down Expand Up @@ -47,11 +46,6 @@ protected void toStoreOnRefState(IcebergView view, ObjectTypes.Content.Builder b
builder.setIcebergViewState(stateBuilder);
}

@Override
public IcebergView applyId(IcebergView content, String id) {
return ((ImmutableIcebergView) content).withId(id);
}

@Override
public boolean requiresGlobalState(ByteString content) {
ObjectTypes.Content parsed = parse(content);
Expand Down