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

Jump to bottom

bugfix: namespaceDepth filter loses entry content #6648

Merged
merged 1 commit into from
Apr 21, 2023
Merged

bugfix: namespaceDepth filter loses entry content #6648

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 15 additions & 7 deletions servers/services/src/main/java/org/projectnessie/services/impl/TreeApiImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -776,11 +776,15 @@ protected Set<Check> checksForEntry(KeyEntry entry) {
continue;
}

Content c = key.getContent();
EntriesResponse.Entry entry =
EntriesResponse.Entry.entry(key.getKey(), key.getType(), key.getContentId());
c != null
? EntriesResponse.Entry.entry(key.getKey(), key.getType(), c)
: EntriesResponse.Entry.entry(key.getKey(), key.getType(), key.getContentId());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

note this is the same logic as in the else branch of the line 768 if statement


entry = namespaceDepthMapping(entry, depth);
entry = maybeTruncateToDepth(entry, depth);

// add implicit namespace entries only once (single parent of multiple real entries)
if (seen.add(entry.getName())) {
if (!pagedResponseHandler.addEntry(entry)) {
pagedResponseHandler.hasMore(authz.tokenForCurrent());
Expand Down Expand Up @@ -815,12 +819,16 @@ protected Set<Check> checksForEntry(KeyEntry entry) {
}
}

private static EntriesResponse.Entry namespaceDepthMapping(
private static EntriesResponse.Entry maybeTruncateToDepth(
EntriesResponse.Entry entry, int depth) {
Content.Type type =
entry.getName().getElementCount() > depth ? Content.Type.NAMESPACE : entry.getType();
ContentKey key = ContentKey.of(entry.getName().getElements().subList(0, depth));
return EntriesResponse.Entry.entry(key, type);
List<String> nameElements = entry.getName().getElements();
boolean truncateToNamespace = nameElements.size() > depth;
if (truncateToNamespace) {
// implicit namespace entry at target depth (virtual parent of real entry)
ContentKey namespaceKey = ContentKey.of(nameElements.subList(0, depth));
return EntriesResponse.Entry.entry(namespaceKey, Content.Type.NAMESPACE);
}
return entry;
}

/**
Expand Down
26 changes: 25 additions & 1 deletion ...s/services/src/testFixtures/java/org/projectnessie/services/impl/AbstractTestEntries.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import static com.google.common.collect.Maps.immutableEntry;
import static java.util.stream.Collectors.toSet;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.tuple;
import static org.junit.jupiter.api.Assumptions.abort;
import static org.projectnessie.model.CommitMeta.fromMessage;
Expand Down Expand Up @@ -341,14 +342,37 @@ public void filterEntriesByNamespaceAndPrefixDepth(ReferenceMode refMode)
immutableEntry(ContentKey.of("a", "b", "fourthTable"), Content.Type.ICEBERG_TABLE),
immutableEntry(ContentKey.of("a", "b", "c"), Content.Type.NAMESPACE));

entries = entries(reference, 4, "entry.namespace.matches('a\\\\.b\\\\.c(\\\\.|$)')");
String filterThatFindsTwoTables = "entry.namespace.matches('a\\\\.b\\\\.c(\\\\.|$)')";
entries = entries(reference, 4, filterThatFindsTwoTables);
soft.assertThat(entries)
.hasSize(2)
.map(e -> immutableEntry(e.getName(), e.getType()))
.containsExactlyInAnyOrder(
immutableEntry(ContentKey.of("a", "b", "c", "secondTable"), Content.Type.ICEBERG_TABLE),
immutableEntry(ContentKey.of("a", "b", "c", "firstTable"), Content.Type.ICEBERG_TABLE));

// namespaceDepth filter always returns contentId of real entries
entries = entries(reference.getName(), reference.getHash(), 4, filterThatFindsTwoTables, false);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

before the fix:

java.lang.AssertionError: 
Expecting all elements of:
  [Entry{type=ICEBERG_TABLE, name=a.b.c.firstTable, contentId=null, content=null},
    Entry{type=ICEBERG_TABLE, name=a.b.c.secondTable, contentId=null, content=null}]
to satisfy given requirements, but these elements did not:

see how both contentId and content were always null

soft.assertThat(entries)
.hasSize(2)
.allSatisfy(
entry -> {
assertThat(entry.getType()).isEqualTo(Content.Type.ICEBERG_TABLE);
assertThat(entry.getContentId()).isNotNull();
assertThat(entry.getContent()).isNull();
});

// namespaceDepth filter returns content of real entries if requested
entries = entries(reference.getName(), reference.getHash(), 4, filterThatFindsTwoTables, true);
soft.assertThat(entries)
.hasSize(2)
.allSatisfy(
entry -> {
assertThat(entry.getType()).isEqualTo(Content.Type.ICEBERG_TABLE);
assertThat(entry.getContentId()).isNotNull();
assertThat(entry.getContent()).isNotNull();
});

entries = entries(reference, 5, "entry.namespace.matches('(\\\\.|$)')");
soft.assertThat(entries).isEmpty();

Expand Down