Skip to content

Commit

Permalink
Issue #21: do not serialize JGit objects in Wicket pages
Browse files Browse the repository at this point in the history 
JGit RevCommits contain parent links, which can lead to deep recusions
during Wicket's page serialization, until a StackOverflowError occurs.
Avoid this by making sure that Wicket objects (pages, panels, or also
models) do not use RevCommit directly but always the Gitblit-own
RepositoryCommit wrapper.

Give RepositoryCommit its own custom serialization which does not write
the RevCommit but only its SHA-1 and upon reading re-fetches the commit
from the repository.

Also ensure that Gitblit's RefModel does not serialize RevObjects.

Adapt callers, where necessary.

Fix occurrences of Wicket DataViews instantiated with RevCommit. Make
those use RepositoryCommit, too.
  • Loading branch information
@tomaswolf
tomaswolf committed Oct 23, 2016
1 parent 53dd054 commit 905230a
Show file tree
Hide file tree
Showing 7 changed files with 175 additions and 55 deletions.
93 changes: 76 additions & 17 deletions src/main/java/com/gitblit/models/RefModel.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,18 +36,42 @@
*/
public class RefModel implements Serializable, Comparable<RefModel> {

private static final long serialVersionUID = 1L;
private static final long serialVersionUID = 876822269940583606L;

public final String displayName;
public final RevObject referencedObject;
public transient Ref reference;

private final Date date;
private final String name;
private final int type;
private final String id;
private final String referencedId;
private final boolean annotated;
private final PersonIdent person;
private final String shortMessage;
private final String fullMessage;

private transient ObjectId objectId;
private transient ObjectId referencedObjectId;

public transient Ref reference; // Used in too many places.

public RefModel(String displayName, Ref ref, RevObject refObject) {
this.displayName = displayName;
this.reference = ref;
this.referencedObject = refObject;
this.displayName = displayName;
this.date = internalGetDate(refObject);
this.name = (ref != null) ? ref.getName() : displayName;
this.type = internalGetReferencedObjectType(refObject);
this.objectId = internalGetObjectId(reference);
this.id = this.objectId.getName();
this.referencedObjectId = internalGetReferencedObjectId(refObject);
this.referencedId = this.referencedObjectId.getName();
this.annotated = internalIsAnnotatedTag(ref, refObject);
this.person = internalGetAuthorIdent(refObject);
this.shortMessage = internalGetShortMessage(refObject);
this.fullMessage = internalGetFullMessage(refObject);
}

public Date getDate() {
private Date internalGetDate(RevObject referencedObject) {
Date date = new Date(0);
if (referencedObject != null) {
if (referencedObject instanceof RevTag) {
Expand All @@ -64,29 +88,41 @@ public Date getDate() {
return date;
}

public Date getDate() {
return date;
}

public String getName() {
if (reference == null) {
return displayName;
}
return reference.getName();
return name;
}

public int getReferencedObjectType() {
private int internalGetReferencedObjectType(RevObject referencedObject) {
int type = referencedObject.getType();
if (referencedObject instanceof RevTag) {
type = ((RevTag) referencedObject).getObject().getType();
}
return type;
}

public ObjectId getReferencedObjectId() {
public int getType() {
return type;
}

private ObjectId internalGetReferencedObjectId(RevObject referencedObject) {
if (referencedObject instanceof RevTag) {
return ((RevTag) referencedObject).getObject().getId();
}
return referencedObject.getId();
}

public String getShortMessage() {
public ObjectId getReferencedObjectId() {
if (referencedObjectId == null) {
referencedObjectId = ObjectId.fromString(referencedId);
}
return referencedObjectId;
}

private String internalGetShortMessage(RevObject referencedObject) {
String message = "";
if (referencedObject instanceof RevTag) {
message = ((RevTag) referencedObject).getShortMessage();
Expand All @@ -96,7 +132,11 @@ public String getShortMessage() {
return message;
}

public String getFullMessage() {
public String getShortMessage() {
return shortMessage;
}

private String internalGetFullMessage(RevObject referencedObject) {
String message = "";
if (referencedObject instanceof RevTag) {
message = ((RevTag) referencedObject).getFullMessage();
Expand All @@ -106,7 +146,11 @@ public String getFullMessage() {
return message;
}

public PersonIdent getAuthorIdent() {
public String getFullMessage() {
return fullMessage;
}

private PersonIdent internalGetAuthorIdent(RevObject referencedObject) {
if (referencedObject instanceof RevTag) {
return ((RevTag) referencedObject).getTaggerIdent();
} else if (referencedObject instanceof RevCommit) {
Expand All @@ -115,17 +159,32 @@ public PersonIdent getAuthorIdent() {
return null;
}

public ObjectId getObjectId() {
public PersonIdent getAuthorIdent() {
return person;
}

private ObjectId internalGetObjectId(Ref reference) {
return reference.getObjectId();
}

public boolean isAnnotatedTag() {
public ObjectId getObjectId() {
if (objectId == null) {
objectId = ObjectId.fromString(id);
}
return objectId;
}

private boolean internalIsAnnotatedTag(Ref reference, RevObject referencedObject) {
if (referencedObject instanceof RevTag) {
return !getReferencedObjectId().equals(getObjectId());
}
return reference.getPeeledObjectId() != null;
}

public boolean isAnnotatedTag() {
return annotated;
}

@Override
public int hashCode() {
return getReferencedObjectId().hashCode() + getName().hashCode();
Expand Down
61 changes: 50 additions & 11 deletions src/main/java/com/gitblit/models/RepositoryCommit.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,30 +15,36 @@
*/
package com.gitblit.models;

import java.io.IOException;
import java.io.ObjectInputStream;
import java.io.ObjectOutputStream;
import java.io.Serializable;
import java.text.MessageFormat;
import java.util.Date;
import java.util.List;

import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;

import com.gitblit.wicket.GitBlitWebApp;

/**
* Model class to represent a RevCommit, it's source repository, and the branch.
* This class is used by the activity page.
* Model class to represent a RevCommit, it's source repository, and the branch. This class is used by the activity page.
*
* @author James Moger
*/
public class RepositoryCommit implements Serializable, Comparable<RepositoryCommit> {

private static final long serialVersionUID = 1L;
private static final long serialVersionUID = -2214911650485772022L;

public final String repository;
public String repository;

public final String branch;
public String branch;

private final RevCommit commit;
private RevCommit commit;

private List<RefModel> refs;

Expand Down Expand Up @@ -80,7 +86,7 @@ public int getParentCount() {
return commit.getParentCount();
}

public RevCommit [] getParents() {
public RevCommit[] getParents() {
return commit.getParents();
}

Expand All @@ -92,10 +98,14 @@ public PersonIdent getCommitterIdent() {
return commit.getCommitterIdent();
}

public RevCommit getCommit() {
return commit;
}

@Override
public boolean equals(Object o) {
if (o instanceof RepositoryCommit) {
RepositoryCommit commit = (RepositoryCommit) o;
final RepositoryCommit commit = (RepositoryCommit) o;
return repository.equals(commit.repository) && getName().equals(commit.getName());
}
return false;
Expand Down Expand Up @@ -123,8 +133,37 @@ public RepositoryCommit clone(String withRef) {

@Override
public String toString() {
return MessageFormat.format("{0} {1} {2,date,yyyy-MM-dd HH:mm} {3} {4}",
getShortName(), branch, getCommitterIdent().getWhen(), getAuthorIdent().getName(),
getShortMessage());
return MessageFormat.format("{0} {1} {2,date,yyyy-MM-dd HH:mm} {3} {4}", getShortName(), branch, getCommitterIdent().getWhen(),
getAuthorIdent().getName(), getShortMessage());
}

// Serialization: do not serialize the JGit RevCommit!

private void writeObject(ObjectOutputStream output) throws IOException {
output.writeObject(repository);
output.writeObject(branch);
output.writeObject(refs);
output.writeObject(commit.getName());
}

@SuppressWarnings("unchecked")
private void readObject(ObjectInputStream input) throws IOException {
try {
repository = (String) input.readObject();
branch = (String) input.readObject();
refs = (List<RefModel>) input.readObject();
final String commitId = (String) input.readObject();
// Go find the commit again.
final Repository repo = GitBlitWebApp.get().repositories().getRepository(repository);
if (repo == null) {
throw new IOException("Cannot find repositoy " + repository);
}
try (RevWalk walk = new RevWalk(repo)) {
commit = walk.parseCommit(repo.resolve(commitId));
}
} catch (final ClassNotFoundException e) {
throw new IOException(e);
}
}

}
2 changes: 1 addition & 1 deletion src/main/java/com/gitblit/wicket/pages/RepositoryPage.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -575,7 +575,7 @@ protected String getShortObjectId(String objectId) {
}

protected void addRefs(Repository r, RevCommit c) {
add(new RefsPanel("refsPanel", repositoryName, c, JGitUtils.getAllRefs(r, getRepositoryModel().showRemoteBranches)));
add(new RefsPanel("refsPanel", repositoryName, JGitUtils.getAllRefs(r, getRepositoryModel().showRemoteBranches).get(c.getId())));
}

protected void addFullText(String wicketId, String text) {
Expand Down
15 changes: 10 additions & 5 deletions src/main/java/com/gitblit/wicket/pages/TicketPage.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@
import com.gitblit.git.PatchsetReceivePack;
import com.gitblit.models.PathModel.PathChangeModel;
import com.gitblit.models.RegistrantAccessPermission;
import com.gitblit.models.RepositoryCommit;
import com.gitblit.models.RepositoryModel;
import com.gitblit.models.RepositoryUrl;
import com.gitblit.models.SubmoduleModel;
Expand Down Expand Up @@ -808,13 +809,17 @@ public void populateItem(final Item<Change> item) {

// commits
List<RevCommit> commits = JGitUtils.getRevLog(getRepository(), currentPatchset.base, currentPatchset.tip);
ListDataProvider<RevCommit> commitsDp = new ListDataProvider<RevCommit>(commits);
DataView<RevCommit> commitsView = new DataView<RevCommit>("commit", commitsDp) {
final List<RepositoryCommit> repoCommits = new ArrayList<>(commits.size());
for (RevCommit c : commits) {
repoCommits.add(new RepositoryCommit(repositoryName, "", c));
}
ListDataProvider<RepositoryCommit> commitsDp = new ListDataProvider<RepositoryCommit>(repoCommits);
DataView<RepositoryCommit> commitsView = new DataView<RepositoryCommit>("commit", commitsDp) {
private static final long serialVersionUID = 1L;

@Override
public void populateItem(final Item<RevCommit> item) {
RevCommit commit = item.getModelObject();
public void populateItem(final Item<RepositoryCommit> item) {
RepositoryCommit commit = item.getModelObject();
PersonIdent author = commit.getAuthorIdent();
item.add(new AvatarImage("authorAvatar", author.getName(), author.getEmailAddress(), null, 16, false));
item.add(new Label("author", commit.getAuthorIdent().getName()));
Expand All @@ -823,7 +828,7 @@ public void populateItem(final Item<RevCommit> item) {
item.add(new LinkPanel("diff", "link", getString("gb.diff"), CommitDiffPage.class,
WicketUtils.newObjectParameter(repositoryName, commit.getName()), true));
item.add(new Label("title", StringUtils.trimString(commit.getShortMessage(), Constants.LEN_SHORTLOG_REFS)));
item.add(WicketUtils.createDateLabel("commitDate", JGitUtils.getAuthorDate(commit), GitBlitWebSession
item.add(WicketUtils.createDateLabel("commitDate", author.getWhen(), GitBlitWebSession
.get().getTimezone(), getTimeUtils(), false));
item.add(new DiffStatPanel("commitDiffStat", 0, 0, true));
}
Expand Down
19 changes: 12 additions & 7 deletions src/main/java/com/gitblit/wicket/panels/HistoryPanel.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
import com.gitblit.models.PathModel;
import com.gitblit.models.PathModel.PathChangeModel;
import com.gitblit.models.RefModel;
import com.gitblit.models.RepositoryCommit;
import com.gitblit.models.SubmoduleModel;
import com.gitblit.utils.JGitUtils;
import com.gitblit.utils.MarkdownUtils;
Expand Down Expand Up @@ -146,15 +147,19 @@ public HistoryPanel(String wicketId, final String repositoryName, final String o
hasMore = commits.size() >= itemsPerPage;

final int hashLen = app().settings().getInteger(Keys.web.shortCommitIdLength, 6);
ListDataProvider<RevCommit> dp = new ListDataProvider<RevCommit>(commits);
DataView<RevCommit> logView = new DataView<RevCommit>("commit", dp) {
final List<RepositoryCommit> repoCommits = new ArrayList<>(commits.size());
for (RevCommit c : commits) {
repoCommits.add(new RepositoryCommit(repositoryName, "", c));
}
ListDataProvider<RepositoryCommit> dp = new ListDataProvider<RepositoryCommit>(repoCommits);
DataView<RepositoryCommit> logView = new DataView<RepositoryCommit>("commit", dp) {
private static final long serialVersionUID = 1L;
int counter;

@Override
public void populateItem(final Item<RevCommit> item) {
final RevCommit entry = item.getModelObject();
final Date date = JGitUtils.getAuthorDate(entry);
public void populateItem(final Item<RepositoryCommit> item) {
final RepositoryCommit entry = item.getModelObject();
Date date = entry.getAuthorIdent().getWhen();

item.add(WicketUtils.createDateLabel("commitDate", date, getTimeZone(), getTimeUtils()));

Expand Down Expand Up @@ -186,7 +191,7 @@ public void populateItem(final Item<RevCommit> item) {
}
item.add(shortlog);

item.add(new RefsPanel("commitRefs", repositoryName, entry, allRefs));
item.add(new RefsPanel("commitRefs", repositoryName, allRefs.get(entry.getId())));

if (isTree) {
// tree
Expand All @@ -204,7 +209,7 @@ public void populateItem(final Item<RevCommit> item) {
} else if (isSubmodule) {
// submodule
Repository repository = app().repositories().getRepository(repositoryName);
String submoduleId = JGitUtils.getSubmoduleCommitId(repository, path, entry);
String submoduleId = JGitUtils.getSubmoduleCommitId(repository, path, entry.getCommit());
repository.close();
if (StringUtils.isEmpty(submoduleId)) {
// not a submodule at this commit, just a matching path
Expand Down
Loading

0 comments on commit 905230a

Please sign in to comment.