Skip to content

Commit

Permalink
Code review rework.
Browse files Browse the repository at this point in the history
  • Loading branch information
@at88mph
at88mph committed Mar 14, 2024
1 parent 712f3ca commit 4fed439
Show file tree
Hide file tree
Showing 5 changed files with 106 additions and 74 deletions.
12 changes: 8 additions & 4 deletions cadc-tap-server/src/main/java/ca/nrc/cadc/tap/BasicUploadManager.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,9 +119,9 @@ public class BasicUploadManager implements UploadManager
protected DateFormat dateFormat;

/**
* Limitations on the UPLOAD VOTable.
* Limitations on the UPLOAD VOTable. Defaults to one Megabyte with unlimited rows and columns.
*/
protected UploadLimits uploadLimits;
protected UploadLimits uploadLimits = new UploadLimits(1024L * 1024L);

protected Job job;

Expand All @@ -132,6 +132,10 @@ private BasicUploadManager() { }

protected BasicUploadManager(UploadLimits uploadLimits)
{
if (uploadLimits == null) {
throw new IllegalStateException("Upload limits are required.");
}

this.uploadLimits = uploadLimits;
dateFormat = DateUtil.getDateFormat(DateUtil.IVOA_DATE_FORMAT, DateUtil.UTC);
}
Expand Down Expand Up @@ -265,8 +269,8 @@ public TableDesc acceptTargetTableDesc(TableDesc td) {
protected VOTableParser getVOTableParser(UploadTable uploadTable)
throws VOTableParserException
{
VOTableParser ret = new JDOMVOTableParser();
ret.setUpload(uploadTable, uploadLimits);
VOTableParser ret = new JDOMVOTableParser(uploadLimits);
ret.setUpload(uploadTable);
return ret;
}

Expand Down
71 changes: 42 additions & 29 deletions cadc-tap-server/src/main/java/ca/nrc/cadc/tap/upload/JDOMVOTableParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@
import ca.nrc.cadc.dali.tables.votable.VOTableResource;
import ca.nrc.cadc.dali.tables.votable.VOTableTable;
import ca.nrc.cadc.io.ByteCountInputStream;
import ca.nrc.cadc.io.ByteLimitExceededException;
import ca.nrc.cadc.tap.UploadManager;
import static ca.nrc.cadc.tap.UploadManager.SCHEMA;
import java.util.ArrayList;
Expand All @@ -85,6 +86,7 @@
import ca.nrc.cadc.tap.schema.TapSchemaUtil;
import java.io.IOException;


/**
* Implements the VOTableParser interface using JDOM.
*
Expand All @@ -98,25 +100,32 @@ public class JDOMVOTableParser implements VOTableParser

protected String tableName;
protected VOTableTable votable;
protected UploadLimits uploadLimits;
protected final UploadLimits uploadLimits;

public JDOMVOTableParser() { }

/**
* Constructor setting limits on upload tables.
* @param uploadLimits Limitations of the Upload table. Required.
*/
public JDOMVOTableParser(UploadLimits uploadLimits) {
if (uploadLimits == null) {
throw new IllegalStateException("Upload limits are required.");
}
this.uploadLimits = uploadLimits;
}

@Override
public void setUpload(UploadTable upload, UploadLimits uploadLimits)
public void setUpload(UploadTable upload)
{
this.upload = upload;
this.uploadLimits = uploadLimits;
}

private void init()
throws IOException
{
if (votable == null)
{
verifyUploadTable();
VOTableReader r = new VOTableReader();
VOTableDocument doc = r.read(upload.uri.toURL().openStream());
VOTableDocument doc = verifyUploadTable();
VOTableResource vr = doc.getResourceByType("results");
this.votable = vr.getTable();
this.tableName = upload.tableName;
Expand All @@ -129,35 +138,39 @@ private void init()
/**
* Ensure the Upload table conforms to specified limitations, if any. This will only read through the file if
* the Upload table file falls into the acceptable size first.
* @throws IOException If any of the limitations are exceeded.
* @throws IOException If the file cannot be read, or if the URI to the Upload file is invalid.
*/
void verifyUploadTable() throws IOException {
VOTableDocument verifyUploadTable() throws IOException {
// Only proceed if a size limitation is set.
if (uploadLimits != null && uploadLimits.byteLimit != null) {
final VOTableReader voTableReader = new VOTableReader();
try (final ByteCountInputStream byteCountInputStream =
new ByteCountInputStream(upload.uri.toURL().openStream(), uploadLimits.byteLimit)) {
final VOTableDocument doc = voTableReader.read(byteCountInputStream);
final VOTableResource vr = doc.getResourceByType("results");
final VOTableTable voTableTable = vr.getTable();
final VOTableReader voTableReader = new VOTableReader();
try (final ByteCountInputStream byteCountInputStream =
new ByteCountInputStream(upload.uri.toURL().openStream(), uploadLimits.byteLimit)) {
final VOTableDocument doc = voTableReader.read(byteCountInputStream);
final VOTableResource vr = doc.getResourceByType("results");
final VOTableTable voTableTable = vr.getTable();

if (uploadLimits.columnLimit != null && voTableTable.getFields().size() > uploadLimits.columnLimit) {
throw new IOException("Column count exceeds maximum of " + uploadLimits.columnLimit);
}
if (uploadLimits.columnLimit != null && voTableTable.getFields().size() > uploadLimits.columnLimit) {
throw new IllegalArgumentException("Column count exceeds maximum of " + uploadLimits.columnLimit);
}

// Avoid iterating if no row limit has been set.
if (uploadLimits.rowLimit != null) {
int counter = 0;
for (final Iterator<List<Object>> iterator = voTableTable.getTableData().iterator();
iterator.hasNext();) {
if (++counter > uploadLimits.rowLimit) {
throw new IOException("Row count exceeds maximum of " + uploadLimits.rowLimit);
} else {
iterator.next();
}
// Avoid iterating if no row limit has been set.
if (uploadLimits.rowLimit != null) {
int counter = 0;
for (final Iterator<List<Object>> iterator = voTableTable.getTableData().iterator();
iterator.hasNext();) {
if (++counter > uploadLimits.rowLimit) {
throw new IllegalArgumentException("Row count exceeds maximum of " + uploadLimits.rowLimit);
} else {
iterator.next();
}
}
}

return doc;
} catch (ByteLimitExceededException byteLimitExceededException) {
throw new IllegalArgumentException("Size of upload file exceeds maximum of "
+ byteLimitExceededException.getLimit() + " bytes.",
byteLimitExceededException);
}
}

Expand Down
14 changes: 8 additions & 6 deletions cadc-tap-server/src/main/java/ca/nrc/cadc/tap/upload/UploadLimits.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,13 +69,15 @@
package ca.nrc.cadc.tap.upload;

public class UploadLimits {
final Long byteLimit;
final Integer rowLimit;
final Integer columnLimit;
final long byteLimit;
public Integer rowLimit;
public Integer columnLimit;

public UploadLimits(Long byteLimit, Integer rowLimit, Integer columnLimit) {
public UploadLimits(long byteLimit) {
if (byteLimit <= 0) {
throw new IllegalStateException(
"Invalid configuration: Upload file size limit should be greater than zero.");
}
this.byteLimit = byteLimit;
this.rowLimit = rowLimit;
this.columnLimit = columnLimit;
}
}
3 changes: 1 addition & 2 deletions cadc-tap-server/src/main/java/ca/nrc/cadc/tap/upload/VOTableParser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,9 @@ public interface VOTableParser
/**
* Set the Upload Table details.
* @param upload The upload table descriptor.
* @param uploadLimits Limitations of the Upload table. Optional.
* @throws VOTableParserException For any parsing exceptions.
*/
void setUpload(UploadTable upload, UploadLimits uploadLimits) throws VOTableParserException;
void setUpload(UploadTable upload) throws VOTableParserException;

/**
* Get a TableDesc of the VOTable.
Expand Down
80 changes: 47 additions & 33 deletions cadc-tap-server/src/test/java/ca/nrc/cadc/tap/upload/JDOMVOTableParserTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,112 +91,126 @@ public class JDOMVOTableParserTest {
}

@Test
public void testUploadValidateUnlimited() throws Exception {
final JDOMVOTableParser testSubject = new JDOMVOTableParser();

final File uploadFile = createUploadFile("testuploadunlimited", "results",
4, 200);

final UploadTable uploadTable = new UploadTable("test1", "jobid1", uploadFile.toURI());
public void testUploadInvalidConditions() {
try {
new JDOMVOTableParser(null);
Assert.fail("Should throw IllegalStateException.");
} catch (IllegalStateException illegalStateException) {
// Good.
}

testSubject.setUpload(uploadTable, null);
testSubject.verifyUploadTable();
try {
new JDOMVOTableParser(new UploadLimits(-3L));
Assert.fail("Should throw IllegalStateException.");
} catch (IllegalStateException illegalStateException) {
// Good.
}
}

@Test
public void testUploadValidateRowLimit() throws Exception {
final JDOMVOTableParser testSubject = new JDOMVOTableParser();
final UploadLimits uploadLimits = new UploadLimits(5L * 1024L * 1024L);
uploadLimits.rowLimit = 2;
final JDOMVOTableParser testSubject = new JDOMVOTableParser(uploadLimits);

final File uploadFile = createUploadFile("testuploadrowlimit", "results",
2, 5);

final UploadTable uploadTable = new UploadTable("test2", "jobid2", uploadFile.toURI());

testSubject.setUpload(uploadTable, new UploadLimits(5L * 1024L * 1024L, 2, null));
testSubject.setUpload(uploadTable);

try {
testSubject.verifyUploadTable();
Assert.fail("Should throw IOException here.");
} catch (IOException ioException) {
Assert.fail("Should throw IllegalArgumentException here.");
} catch (IllegalArgumentException ioException) {
Assert.assertEquals("Wrong message.", "Row count exceeds maximum of 2",
ioException.getMessage());
}
}

@Test
public void testUploadValidateColumnLimit() throws Exception {
final JDOMVOTableParser testSubject = new JDOMVOTableParser();
final UploadLimits uploadLimits = new UploadLimits(5L * 1024L * 1024L);
uploadLimits.columnLimit = 4;
final JDOMVOTableParser testSubject = new JDOMVOTableParser(uploadLimits);

final File uploadFile = createUploadFile("testuploadcolumnlimit", "results",
12, 500);

final UploadTable uploadTable = new UploadTable("test3", "jobid3", uploadFile.toURI());

testSubject.setUpload(uploadTable, new UploadLimits(5L * 1024L * 1024L, null, 4));
testSubject.setUpload(uploadTable);

try {
testSubject.verifyUploadTable();
Assert.fail("Should throw IOException here.");
} catch (IOException ioException) {
Assert.fail("Should throw IllegalArgumentException here.");
} catch (IllegalArgumentException ioException) {
Assert.assertEquals("Wrong message.", "Column count exceeds maximum of 4",
ioException.getMessage());
}
}

@Test
public void testUploadValidateByteLimit() throws Exception {
final JDOMVOTableParser testSubject = new JDOMVOTableParser();
final JDOMVOTableParser testSubject = new JDOMVOTableParser(new UploadLimits(1024L));

final File uploadFile = createUploadFile("testuploadbytelimit", "results",
12, 5000);

final UploadTable uploadTable = new UploadTable("test4", "jobid4", uploadFile.toURI());

testSubject.setUpload(uploadTable, new UploadLimits(1024L, null, null));
testSubject.setUpload(uploadTable);

try {
testSubject.verifyUploadTable();
Assert.fail("Should throw ByteLimitExceededException here.");
} catch (ByteLimitExceededException ioException) {
Assert.assertEquals("Wrong message.", 1024L, ioException.getLimit(), 0L);
Assert.fail("Should throw IllegalArgumentException here.");
} catch (IllegalArgumentException ioException) {
Assert.assertEquals("Wrong message.", 1024L,
((ByteLimitExceededException) ioException.getCause()).getLimit(), 0L);
}
}

@Test
public void testUploadValidateCombinationLimit1() throws Exception {
final JDOMVOTableParser testSubject = new JDOMVOTableParser();
final UploadLimits uploadLimits = new UploadLimits(1024L * 1024L);
uploadLimits.rowLimit = 2000;
final JDOMVOTableParser testSubject = new JDOMVOTableParser(uploadLimits);

final File uploadFile = createUploadFile("testuploadcombinationlimit2", "results",
12, 5000);

final UploadTable uploadTable = new UploadTable("test5", "jobid5", uploadFile.toURI());

testSubject.setUpload(uploadTable, new UploadLimits(1024L * 1024L, 2000, null));
testSubject.setUpload(uploadTable);

try {
testSubject.verifyUploadTable();
Assert.fail("Should throw IOException here.");
} catch (ByteLimitExceededException ioException) {
Assert.assertEquals("Wrong message.", 1024L * 1024L, ioException.getLimit(), 0L);
Assert.fail("Should throw IllegalArgumentException here.");
} catch (IllegalArgumentException ioException) {
Assert.assertEquals("Wrong message.", 1024L * 1024L,
((ByteLimitExceededException) ioException.getCause()).getLimit(), 0L);
}
}

@Test
public void testUploadValidateCombinationLimit2() throws Exception {
final JDOMVOTableParser testSubject = new JDOMVOTableParser();
final UploadLimits uploadLimits = new UploadLimits(5L * 1024L * 1024L);
uploadLimits.rowLimit = 2000;
uploadLimits.columnLimit = 10;
final JDOMVOTableParser testSubject = new JDOMVOTableParser(uploadLimits);

final File uploadFile = createUploadFile("testuploadcombinationlimit2", "results",
12, 5000);

final UploadTable uploadTable = new UploadTable("test6", "jobid6", uploadFile.toURI());

testSubject.setUpload(uploadTable, new UploadLimits(5 * 1024L * 1024L, 2000, null));
testSubject.setUpload(uploadTable);

try {
testSubject.verifyUploadTable();
Assert.fail("Should throw IOException here.");
} catch (IOException ioException) {
Assert.assertEquals("Wrong message.", "Row count exceeds maximum of 2000",
Assert.fail("Should throw IllegalArgumentException here.");
} catch (IllegalArgumentException ioException) {
Assert.assertEquals("Wrong message.", "Column count exceeds maximum of 10",
ioException.getMessage());
}
}
Expand Down

0 comments on commit 4fed439

Please sign in to comment.