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

feat(bookmarks): Permit references to tags from previous invocations #7

Merged
merged 2 commits into from
Jun 12, 2024
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions src/main/java/com/github/olivergondza/saxeed/Bookmark.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package com.github.olivergondza.saxeed;

/**
* A reference to a tag acquired in a previous Saxeed run, to be queried in the next run.
*
* The reference is valid only between two consecutive runs. Provided the document was modified between the executions,
* Saxeed provides no guarantee it will match anything, fail predictably, or match the intended tag.
*
* To bookmark an element, call {@link Tag#bookmark()}. The object returned is valid outside the Saxeed transformation.
* In the next processing, use {@link Tag#isBookmarked(Bookmark)} or {@link Tag#isBookmarked(java.util.List)} to
* identify the bookmarked element.
*/
public interface Bookmark {

/**
* Determine if the tag was removed from the output.
*/
boolean isOmitted();
}
18 changes: 17 additions & 1 deletion src/main/java/com/github/olivergondza/saxeed/Tag.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.github.olivergondza.saxeed;

import java.util.Collection;
import java.util.List;
import java.util.Map;

/**
Expand Down Expand Up @@ -65,6 +66,21 @@ public interface Tag {
*/
Map<String, String> getAttributes();

/**
* Create a bookmark for this element.
*/
Bookmark bookmark();

/**
* Determine if this tag matches the bookmark provided.
*/
boolean isBookmarked(Bookmark bookmark);

/**
* Determine if this tag has been bookmarked by any of the bookmarks provided.
*/
boolean isBookmarked(List<Bookmark> bookmarks);

/**
* Determine if the current tag was added by a visitor.
*
Expand All @@ -76,7 +92,7 @@ public interface Tag {
boolean isGenerated();

/**
* Determine if the current was removed.
* Determine if the current tag was removed.
*
* In other words, it will not be written to the Target.
*/
Expand Down
14 changes: 14 additions & 0 deletions src/main/java/com/github/olivergondza/saxeed/TagName.java
Original file line number Diff line number Diff line change
Expand Up @@ -100,4 +100,18 @@ public String getQualifiedName() {
public TagName inheritNamespace(String name) {
return new TagName(uri, prefix, name);
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;

TagName tagName = (TagName) o;
return Objects.equals(local, tagName.local) && Objects.equals(uri, tagName.uri);
}
olivergondza marked this conversation as resolved.
Show resolved Hide resolved

@Override
public int hashCode() {
return Objects.hash(local, uri);
}
olivergondza marked this conversation as resolved.
Show resolved Hide resolved
}
11 changes: 0 additions & 11 deletions src/main/java/com/github/olivergondza/saxeed/Transformation.java

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
package com.github.olivergondza.saxeed.internal;

import com.github.olivergondza.saxeed.Bookmark;
import com.github.olivergondza.saxeed.TagName;

import java.util.Objects;

public class BookmarkImpl implements Bookmark {
private String value;
private boolean omitted = false;

static BookmarkImpl from(BookmarkImpl parent, TagName name, int count) {
return new BookmarkImpl(pathFrom(parent, name, count));
}

static String pathFrom(BookmarkImpl parent, TagName name, int count) {
StringBuilder sb = new StringBuilder();
if (parent != null) {
assert parent.value != null: "Parent tag " + parent + " must not be written by the time its children " + name + " are still being created.";
sb.append(parent.value);
}

sb.append('/').append(name.getLocal());
String nsUri = name.getNsUri();
if (!nsUri.isEmpty()) {
sb.append('<').append(nsUri).append('>');
}
sb.append('[').append(count).append(']');
return sb.toString();
}

private BookmarkImpl(String bookmark) {
this.value = bookmark;
}

/*package*/ void update(String value) {
this.value = value;
}

/*package*/ void omit() {
omitted = true;
}

@Override
public boolean isOmitted() {
return omitted;
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;

BookmarkImpl bookmark = (BookmarkImpl) o;

// If either is omitted, they are not equal. This is to prevent that omitted bookmark would match real tag based
// on value clash.
if (omitted || bookmark.omitted) {
return false;
}

return Objects.equals(value, bookmark.value);
}

@Override
public int hashCode() {
return Objects.hash(value, omitted);
}
Comment on lines +50 to +68

Choose a reason for hiding this comment

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

The equals and hashCode methods are not consistent. The equals method considers both value and omitted fields, while the hashCode method only considers the value field. This inconsistency can lead to unexpected behavior when instances of BookmarkImpl are used in collections that rely on these methods.

Recommendation: Ensure that both equals and hashCode methods consider the same fields. Either include omitted in the hashCode method or exclude it from the equals method.


@Override
public String toString() {
return value;
}
}
59 changes: 52 additions & 7 deletions src/main/java/com/github/olivergondza/saxeed/internal/TagImpl.java
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
package com.github.olivergondza.saxeed.internal;

import com.github.olivergondza.saxeed.Bookmark;
import com.github.olivergondza.saxeed.Tag;
import com.github.olivergondza.saxeed.TagName;
import org.xml.sax.Attributes;

import java.util.*;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.Consumer;
import java.util.stream.Collectors;

Expand Down Expand Up @@ -57,42 +59,58 @@
*/
private TagImpl wrapWith;

private final BookmarkImpl bookmark;
private final Map<TagName, AtomicInteger> childCounts = new HashMap<>();

/**
* Create generated Tag.
*/
private TagImpl(TagImpl parent, TagName name) {
this.parent = parent;
this.name = Objects.requireNonNull(name);
this.attrs = null; // No SAX attrs, setting attributes right away
// No SAX attrs, setting attributes right away
this.attrs = null;
this.attributes = new LinkedHashMap<>();
this.namespaces = null;
this.generated = true;
this.bookmark = initBookmark();
init(parent);
}
Comment on lines 68 to 78

Choose a reason for hiding this comment

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

The constructor TagImpl(TagImpl parent, TagName name) initializes the attributes and namespaces fields to empty collections or null. This can lead to potential NullPointerException issues if these fields are accessed without null checks elsewhere in the code. Consider initializing these fields to empty collections to avoid such issues.


/**
* Create Tag from input.
*/
public TagImpl(TagImpl parent, TagName name, Attributes attrs, Map<String, String> namespaces) {
this.parent = parent;
this.name = Objects.requireNonNull(name);
this.attrs = Objects.requireNonNull(attrs);
// Create defensive copy in either case
this.namespaces = namespaces.isEmpty() ? null : new LinkedHashMap<>(namespaces);
this.generated = false;
this.bookmark = initBookmark();
init(parent);
}
Comment on lines 83 to 92

Choose a reason for hiding this comment

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

The constructor TagImpl(TagImpl parent, TagName name, Attributes attrs, Map<String, String> namespaces) does not perform a null check on the namespaces parameter before calling namespaces.isEmpty(). This can lead to a NullPointerException if namespaces is null. Consider adding a null check for the namespaces parameter before using it.


private void init(TagImpl parent) {
this.parent = parent;

// Inherit the write mode based on the parent's one.
if (parent != null) {
// Inherit the write mode based on the parent's one.
writeMode = parent.writeMode.children;
}

// Verify invariant
traverseParentChain(null);
}

private BookmarkImpl initBookmark() {
if (parent != null) {
assert parent.bookmark != null;
AtomicInteger parentChildCount = parent.childCounts.computeIfAbsent(name, k -> new AtomicInteger());
return BookmarkImpl.from(parent.bookmark, name, parentChildCount.getAndIncrement());
} else {
return BookmarkImpl.from(null, name, 0);
}
}

@Override
public Map<String, String> getAttributes() {
if (attributes == null) {
Expand Down Expand Up @@ -122,16 +140,30 @@ public boolean isNamed(String name) {
*/
@Override
public boolean isNamed(TagName name) {
return Objects.equals(name.getLocal(), this.name.getLocal())
&& Objects.equals(name.getNsUri(), this.name.getNsUri())
;
return Objects.equals(name, this.name);
}

@Override
public boolean isGenerated() {
return generated;
}

@Override
public boolean isBookmarked(Bookmark bookmark) {
Objects.requireNonNull(bookmark, "null bookmark provided");
return this.bookmark.equals(bookmark);
}

@Override
public boolean isBookmarked(List<Bookmark> bookmarks) {
for (Bookmark bookmark : bookmarks) {
if (isBookmarked(bookmark)) {
return true;
}
}
return false;
olivergondza marked this conversation as resolved.
Show resolved Hide resolved
}

@Override
public TagName getName() {
return name;
Expand Down Expand Up @@ -169,6 +201,19 @@ public Tag getAncestor(TagName name) {
return null;
}

@Override
public Bookmark bookmark() {
return bookmark;
}

/*package*/ BookmarkImpl getBookmark() {
return bookmark;
}

/*package*/ void bookmarkWrittenAs(String newPath) {
bookmark.update(newPath);
}

@Override
public TagImpl addChild(String name) {
return addChild(this.name.inheritNamespace(name));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.logging.Logger;
import java.util.stream.Collectors;

Expand All @@ -40,9 +41,12 @@ public class TransformationHandler extends DefaultHandler implements AutoCloseab

private TagImpl currentTag;
private final CharChunk currentChars = new CharChunk();
private LinkedHashMap<String, String> currentNsMapping = new LinkedHashMap<>();

private Map<String, String> documentNamespaces = new HashMap<>();
private final LinkedHashMap<String, String> currentNsMapping = new LinkedHashMap<>();

private final Map<String, String> documentNamespaces = new HashMap<>();

private final Map<String, AtomicInteger> writtenBookmarks = new HashMap<>();

public TransformationHandler(
LinkedHashMap<UpdatingVisitor, Subscribed> visitors,
Expand All @@ -61,22 +65,30 @@ public void startPrefixMapping(String prefix, String uri) {
}

@Override
public void startElement(String uri, String localName, String tagName, Attributes attributes) {
TagName tagname = TagName.fromSaxArgs(uri, localName, tagName);
currentTag = new TagImpl(currentTag, tagname, attributes, currentNsMapping);
public void startElement(String uri, String localName, String qName, Attributes attributes) {
TagName tagName = TagName.fromSaxArgs(uri, localName, qName);
TagImpl parent = currentTag;

currentTag = new TagImpl(parent, tagName, attributes, currentNsMapping);
currentNsMapping.clear();

_startElement(currentTag);
}
olivergondza marked this conversation as resolved.
Show resolved Hide resolved

private void _startElement(TagImpl tag) {
if (tag.isOmitted()) return;
if (tag.isOmitted()) {
tag.getBookmark().omit();
return;
}

TagName name = tag.getName();
for (UpdatingVisitor v : getVisitors(name)) {
v.startTag(tag);

if (tag.isOmitted()) return;
if (tag.isOmitted()) {
tag.getBookmark().omit();
return;
}
}

TagImpl wrapper = tag.startWrapWith();
Expand Down Expand Up @@ -132,12 +144,15 @@ private void _startElement(TagImpl tag) {
}
LOGGER.fine(">");

tag.bookmarkWrittenAs(getWriteBookmarkPath(tag));

writeChildren(tag);
} catch (XMLStreamException e) {
throw new FailedWriting(ERROR_WRITING_TO_OUTPUT_FILE, e);
}
}


/**
* Write namespace declarations ("xmlns" pseudo-attributes), existing or added
*/
Expand All @@ -147,6 +162,17 @@ private void writeNamespaceDeclarations(TagImpl tag) throws XMLStreamException {
}
}

private String getWriteBookmarkPath(TagImpl tag) {
TagImpl parent = (TagImpl) tag.getParent();

BookmarkImpl parentBookmark = parent == null ? null : parent.getBookmark();

String key = BookmarkImpl.pathFrom(parentBookmark, tag.getName(), -1);
AtomicInteger counter = writtenBookmarks.computeIfAbsent(key, k -> new AtomicInteger(0));

return BookmarkImpl.pathFrom(parentBookmark, tag.getName(), counter.getAndIncrement());
olivergondza marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* Get visitors subscribed to given tag name.
*
Expand Down
Loading