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

API and JavaDoc changes for Spatial Datatypes #752

Merged
merged 28 commits into from
Jul 27, 2018
Merged

API and JavaDoc changes for Spatial Datatypes #752

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
73edf05
Added more exceptions for fuzz testing, and added some more javadocs
peterbae Jul 18, 2018
a09c247
fixing merge break
peterbae Jul 18, 2018
3f694ff
removing log files
peterbae Jul 18, 2018
243f294
javadoc changes
peterbae Jul 19, 2018
537959f
gets -> returns
peterbae Jul 19, 2018
862d71d
add test
peterbae Jul 20, 2018
d86b237
split points into two
peterbae Jul 20, 2018
eac074f
move error message around
peterbae Jul 20, 2018
79c7e59
check neg size for all number of sizes
peterbae Jul 20, 2018
81a065f
reflect comments + make internal spatial datatype file protected
peterbae Jul 23, 2018
7ec6fcc
more javadoc changes
peterbae Jul 23, 2018
272a729
switch lat/long
peterbae Jul 23, 2018
54c5297
refactor out duplicate code
peterbae Jul 23, 2018
a3eb552
add null checking + change int to long
peterbae Jul 23, 2018
0de6184
handle m and z value being null case and add test
peterbae Jul 23, 2018
b4fca56
reuse function
peterbae Jul 23, 2018
3135521
move this part to other PR
peterbae Jul 24, 2018
2dfb95b
reflect comment
peterbae Jul 24, 2018
6f4af3d
bit more javadoc
peterbae Jul 24, 2018
8aba9e6
remove StringIndexOutOfBoundsException
peterbae Jul 25, 2018
589c1c0
check wkt length
peterbae Jul 25, 2018
a61a58b
Merge branch 'dev' into Fuzz_And_Javadoc_fix
peterbae Jul 25, 2018
1d7800d
remove catching runtime exception
peterbae Jul 26, 2018
1f3e437
Merge branch 'Fuzz_And_Javadoc_fix' of https://github.com/peterbae/ms…
peterbae Jul 26, 2018
35d99a8
dont need those imports
peterbae Jul 26, 2018
e156263
use try blocks they're the best
peterbae Jul 26, 2018
5ca2a83
refactor throwing illegal WKT position into a method
peterbae Jul 26, 2018
2f886a0
apparently 0 and null are different, so fix that
peterbae Jul 26, 2018
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
50 changes: 32 additions & 18 deletions src/main/java/com/microsoft/sqlserver/jdbc/Geography.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,16 @@

package com.microsoft.sqlserver.jdbc;

import java.nio.BufferUnderflowException;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need to import this?

import java.nio.ByteBuffer;
import java.nio.ByteOrder;
import java.text.MessageFormat;


/**
* Geography datatype represents data in a round-earth coordinate system.
*/

public class Geography extends SQLServerSpatialDatatype {
Copy link
Contributor

Choose a reason for hiding this comment

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

needs class description


/**
Expand Down Expand Up @@ -49,7 +55,13 @@ private Geography(byte[] wkb) throws SQLServerException {
buffer = ByteBuffer.wrap(wkb);
buffer.order(ByteOrder.LITTLE_ENDIAN);

parseWkb();
try {
parseWkb();
} catch (BufferUnderflowException e) {
MessageFormat form = new MessageFormat(SQLServerException.getErrString("R_ParsingError"));
Object[] msgArgs = {JDBCType.VARBINARY};
throw new SQLServerException(this, form.format(msgArgs), null, 0, false);
}

WKTsb = new StringBuffer();
WKTsbNoZM = new StringBuffer();
Expand All @@ -62,8 +74,8 @@ private Geography(byte[] wkb) throws SQLServerException {
}

/**
* Returns a Geography instance from an Open Geospatial Consortium (OGC) Well-Known Text (WKT) representation
* augmented with any Z (elevation) and M (measure) values carried by the instance.
* Constructor for a Geography instance from an Open Geospatial Consortium (OGC) Well-Known Text (WKT)
* representation augmented with any Z (elevation) and M (measure) values carried by the instance.
*
* @param wkt
* Well-Known Text (WKT) provided by the user.
Expand All @@ -78,7 +90,8 @@ public static Geography STGeomFromText(String wkt, int srid) throws SQLServerExc
}

/**
* Returns a Geography instance from an Open Geospatial Consortium (OGC) Well-Known Binary (WKB) representation.
* Constructor for a Geography instance from an Open Geospatial Consortium (OGC) Well-Known Binary (WKB)
* representation.
*
* @param wkb
* Well-Known Binary (WKB) provided by the user.
Expand All @@ -91,7 +104,7 @@ public static Geography STGeomFromWKB(byte[] wkb) throws SQLServerException {
}

/**
* Returns a constructed Geography from an internal SQL Server format for spatial data.
* Constructor for a Geography instance from an internal SQL Server format for spatial data.
*
* @param wkb
* Well-Known Binary (WKB) provided by the user.
Expand All @@ -104,8 +117,8 @@ public static Geography deserialize(byte[] wkb) throws SQLServerException {
}

/**
* Returns a Geography instance from an Open Geospatial Consortium (OGC) Well-Known Text (WKT) representation. SRID
* is defaulted to 4326.
* Constructor for a Geography instance from an Open Geospatial Consortium (OGC) Well-Known Text (WKT)
* representation. SRID is defaulted to 4326.
*
* @param wkt
* Well-Known Text (WKT) provided by the user.
Expand All @@ -118,7 +131,7 @@ public static Geography parse(String wkt) throws SQLServerException {
}

/**
* Constructs a Geography instance that represents a Point instance from its X and Y values and an SRID.
* Constructor for a Geography instance that represents a Point instance from its X and Y values and an SRID.
Copy link
Member

Choose a reason for hiding this comment

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

SRID = Spatial Reference System Identifier
Also add to the param tags in all functions

*
* @param x
* x coordinate
Expand Down Expand Up @@ -203,8 +216,8 @@ public boolean hasZ() {
* @return double value that represents the X coordinate.
Copy link
Member

Choose a reason for hiding this comment

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

Please fix Javadocs too. Also for getY() function

*/
public Double getX() {
Copy link
Member

@cheenamalhotra cheenamalhotra Jul 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Please rename getX() and getY() to getLatitude() and getLongitude() for Geography.
A Geographic Point should always reference them as Latitude and Longitude. There should be no API neither documentation for these co-ordinates as X or Y in context of Geography Point.

if (null != internalType && internalType == InternalSpatialDatatype.POINT && points.length == 2) {
return points[0];
if (null != internalType && internalType == InternalSpatialDatatype.POINT && xpoints.length == 1) {
return xpoints[0];
}
return null;
}
Expand All @@ -215,8 +228,8 @@ public Double getX() {
* @return double value that represents the Y coordinate.
*/
public Double getY() {
if (null != internalType && internalType == InternalSpatialDatatype.POINT && points.length == 2) {
return points[1];
if (null != internalType && internalType == InternalSpatialDatatype.POINT && ypoints.length == 1) {
return ypoints[0];
}
return null;
}
Expand Down Expand Up @@ -317,8 +330,8 @@ protected void serializeToWkb(boolean noZM) {
}

for (int i = 0; i < numberOfPoints; i++) {
buf.putDouble(points[2 * i + 1]);
buf.putDouble(points[2 * i]);
buf.putDouble(ypoints[i]);
buf.putDouble(xpoints[i]);
}

if (!noZM) {
Expand Down Expand Up @@ -368,7 +381,7 @@ protected void serializeToWkb(boolean noZM) {
return;
}

protected void parseWkb() {
protected void parseWkb() throws SQLServerException {
srid = buffer.getInt();
version = buffer.get();
serializationProperties = buffer.get();
Expand Down Expand Up @@ -403,10 +416,11 @@ protected void parseWkb() {
}

private void readPoints() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a few methods are duplicated in both Geometry/Geography classes and they should be moved to SQLServerSpatialDatatype.

  1. parseWkb(), exact duplicate.
  2. readPoints(), serializeToWkb() - The only difference is you have to read yValues before xValues. Can you add some kind of a check and move these methods to SQLServerSpatialDatatype too?
  3. There is a lot of duplication in other methods too, please take a look and move to SQLServerSpatialDatatype when possible.

points = new double[2 * numberOfPoints];
xpoints = new double[numberOfPoints];
ypoints = new double[numberOfPoints];
for (int i = 0; i < numberOfPoints; i++) {
points[2 * i + 1] = buffer.getDouble();
points[2 * i] = buffer.getDouble();
ypoints[i] = buffer.getDouble();
xpoints[i] = buffer.getDouble();
}
}
}
48 changes: 31 additions & 17 deletions src/main/java/com/microsoft/sqlserver/jdbc/Geometry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,16 @@

package com.microsoft.sqlserver.jdbc;

import java.nio.BufferUnderflowException;
import java.nio.ByteBuffer;
import java.nio.ByteOrder;
import java.text.MessageFormat;


/**
* Geometry datatype represents data in a Euclidean (flat) coordinate system.
*/

public class Geometry extends SQLServerSpatialDatatype {

/**
Expand Down Expand Up @@ -49,7 +55,13 @@ private Geometry(byte[] wkb) throws SQLServerException {
buffer = ByteBuffer.wrap(wkb);
buffer.order(ByteOrder.LITTLE_ENDIAN);

parseWkb();
try {
parseWkb();
} catch (BufferUnderflowException e) {
MessageFormat form = new MessageFormat(SQLServerException.getErrString("R_ParsingError"));
Object[] msgArgs = {JDBCType.VARBINARY};
throw new SQLServerException(this, form.format(msgArgs), null, 0, false);
}

WKTsb = new StringBuffer();
WKTsbNoZM = new StringBuffer();
Expand All @@ -62,7 +74,7 @@ private Geometry(byte[] wkb) throws SQLServerException {
}

/**
* Returns a Geometry instance from an Open Geospatial Consortium (OGC) Well-Known Text (WKT) representation
* Constructor for a Geometry instance from an Open Geospatial Consortium (OGC) Well-Known Text (WKT) representation
* augmented with any Z (elevation) and M (measure) values carried by the instance.
*
* @param wkt
Expand All @@ -78,7 +90,8 @@ public static Geometry STGeomFromText(String wkt, int srid) throws SQLServerExce
}

/**
* Returns a Geometry instance from an Open Geospatial Consortium (OGC) Well-Known Binary (WKB) representation.
* Constructor for a Geometry instance from an Open Geospatial Consortium (OGC) Well-Known Binary (WKB)
* representation.
*
* @param wkb
* Well-Known Binary (WKB) provided by the user.
Expand All @@ -91,7 +104,7 @@ public static Geometry STGeomFromWKB(byte[] wkb) throws SQLServerException {
}

/**
* Returns a constructed Geometry from an internal SQL Server format for spatial data.
* Constructor for a Geometry instance from an internal SQL Server format for spatial data.
*
* @param wkb
* Well-Known Binary (WKB) provided by the user.
Expand All @@ -104,8 +117,8 @@ public static Geometry deserialize(byte[] wkb) throws SQLServerException {
}

/**
* Returns a Geometry instance from an Open Geospatial Consortium (OGC) Well-Known Text (WKT) representation. SRID
* is defaulted to 0.
* Constructor for a Geometry instance from an Open Geospatial Consortium (OGC) Well-Known Text (WKT)
* representation. SRID is defaulted to 0.
*
* @param wkt
Copy link
Member

Choose a reason for hiding this comment

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

This method has param name as wkt, the other method has wellKnownText, can we generalize and use same name for all similar parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

* Well-Known Text (WKT) provided by the user.
Expand All @@ -118,7 +131,7 @@ public static Geometry parse(String wkt) throws SQLServerException {
}

/**
* Constructs a Geometry instance that represents a Point instance from its X and Y values and an SRID.
* Constructor for a Geometry instance that represents a Point instance from its X and Y values and an SRID.
*
* @param x
* x coordinate
Expand Down Expand Up @@ -203,8 +216,8 @@ public boolean hasZ() {
* @return double value that represents the X coordinate.
*/
public Double getX() {
if (null != internalType && internalType == InternalSpatialDatatype.POINT && points.length == 2) {
return points[0];
if (null != internalType && internalType == InternalSpatialDatatype.POINT && xpoints.length == 1) {
return xpoints[0];
}
return null;
}
Expand All @@ -215,8 +228,8 @@ public Double getX() {
* @return double value that represents the Y coordinate.
*/
public Double getY() {
if (null != internalType && internalType == InternalSpatialDatatype.POINT && points.length == 2) {
return points[1];
if (null != internalType && internalType == InternalSpatialDatatype.POINT && ypoints.length == 1) {
return ypoints[0];
}
return null;
}
Expand Down Expand Up @@ -317,8 +330,8 @@ protected void serializeToWkb(boolean noZM) {
}

for (int i = 0; i < numberOfPoints; i++) {
buf.putDouble(points[2 * i]);
buf.putDouble(points[2 * i + 1]);
buf.putDouble(xpoints[i]);
buf.putDouble(ypoints[i]);
}

if (!noZM) {
Expand Down Expand Up @@ -369,7 +382,7 @@ protected void serializeToWkb(boolean noZM) {
return;
}

protected void parseWkb() {
protected void parseWkb() throws SQLServerException {
srid = buffer.getInt();
version = buffer.get();
serializationProperties = buffer.get();
Expand Down Expand Up @@ -404,10 +417,11 @@ protected void parseWkb() {
}

private void readPoints() {
points = new double[2 * numberOfPoints];
xpoints = new double[numberOfPoints];
ypoints = new double[numberOfPoints];
for (int i = 0; i < numberOfPoints; i++) {
points[2 * i] = buffer.getDouble();
points[2 * i + 1] = buffer.getDouble();
xpoints[i] = buffer.getDouble();
ypoints[i] = buffer.getDouble();
}
}
}
6 changes: 4 additions & 2 deletions src/main/java/com/microsoft/sqlserver/jdbc/SQLServerPreparedStatement.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2010,7 +2010,8 @@ public int[] executeBatch() throws SQLServerException, BatchUpdateException, SQL
} catch (SQLException e) {
// throw a BatchUpdateException with the given error message, and return null for the updateCounts.
throw new BatchUpdateException(e.getMessage(), null, 0, null);
} catch (IllegalArgumentException e) {
}
catch (IllegalArgumentException | StringIndexOutOfBoundsException e) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this change is not related to Spatial types? You probably need to update the comment below too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup

Copy link
Member

Choose a reason for hiding this comment

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

Please move this change out of this PR as we already have another active PR to track useBulkCopyWithBatchInsert API changes. It will be easier to track changes there.

// If we fail with IllegalArgumentException, fall back to the original batch insert logic.
if (getStatementLogger().isLoggable(java.util.logging.Level.FINE)) {
getStatementLogger().fine("Parsing user's Batch Insert SQL Query failed: " + e.toString());
Expand Down Expand Up @@ -2154,7 +2155,8 @@ public long[] executeLargeBatch() throws SQLServerException, BatchUpdateExceptio
} catch (SQLException e) {
// throw a BatchUpdateException with the given error message, and return null for the updateCounts.
throw new BatchUpdateException(e.getMessage(), null, 0, null);
} catch (IllegalArgumentException e) {
}
catch (IllegalArgumentException | StringIndexOutOfBoundsException e) {
// If we fail with IllegalArgumentException, fall back to the original batch insert logic.
if (getStatementLogger().isLoggable(java.util.logging.Level.FINE)) {
getStatementLogger().fine("Parsing user's Batch Insert SQL Query failed: " + e.toString());
Expand Down
Loading