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

API and JavaDoc changes for Spatial Datatypes #752

Merged
merged 28 commits into from
Jul 27, 2018

Conversation

peterbae
Copy link
Contributor

Changes needed after fuzz testing.

@codecov-io
Copy link

codecov-io commented Jul 19, 2018

Codecov Report

Merging #752 into dev will decrease coverage by 0.13%.
The diff coverage is 70.64%.

Impacted file tree graph

@@             Coverage Diff              @@
##                dev     #752      +/-   ##
============================================
- Coverage     48.32%   48.19%   -0.14%     
+ Complexity     2773     2765       -8     
============================================
  Files           116      116              
  Lines         27871    27831      -40     
  Branches       4631     4624       -7     
============================================
- Hits          13469    13412      -57     
- Misses        12218    12229      +11     
- Partials       2184     2190       +6
Flag Coverage Δ Complexity Δ
#JDBC42 47.73% <70.64%> (+0.01%) 2724 <65> (+2) ⬆️
#JDBC43 48.05% <70.64%> (-0.21%) 2756 <65> (-15)
Impacted Files Coverage Δ Complexity Δ
...rosoft/sqlserver/jdbc/InternalSpatialDatatype.java 91.3% <ø> (ø) 6 <0> (ø) ⬇️
...oft/sqlserver/jdbc/SQLServerPreparedStatement.java 54.11% <0%> (ø) 211 <0> (ø) ⬇️
...in/java/com/microsoft/sqlserver/jdbc/Geometry.java 50.79% <53.33%> (-21.65%) 14 <3> (-19)
...n/java/com/microsoft/sqlserver/jdbc/Geography.java 50.79% <56.25%> (-19.29%) 14 <4> (-19)
...osoft/sqlserver/jdbc/SQLServerSpatialDatatype.java 85.38% <74.4%> (-0.88%) 290 <58> (+33)
...rc/main/java/com/microsoft/sqlserver/jdbc/DDC.java 43.1% <0%> (-2.19%) 105% <0%> (-2%)
...m/microsoft/sqlserver/jdbc/SQLServerException.java 75.75% <0%> (-1.52%) 30% <0%> (-1%)
...rc/main/java/com/microsoft/sqlserver/jdbc/dtv.java 62.2% <0%> (-0.35%) 0% <0%> (ø)
...om/microsoft/sqlserver/jdbc/SQLServerBulkCopy.java 52.54% <0%> (-0.18%) 262% <0%> (-2%)
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d0f906...2f886a0. Read the comment docs.

@@ -1662,14 +1662,30 @@ private void skipWhiteSpaces() {
this.pointOffset = pointOffset;
}

/**
* Returns the figuresAttribute value.
Copy link
Contributor

Choose a reason for hiding this comment

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

I am changing everything to be consistent. For getters, can you please say "Gets blah blah"?

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


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

@@ -198,7 +211,7 @@ public boolean hasZ() {
}

/**
* Returns the X coordinate value.
* Gets the X coordinate value.
Copy link
Contributor

Choose a reason for hiding this comment

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

I am sorry I made you change.....we're now going with "Returns the something of this thing." as per new guideline. You can leave it if you want I'm changing all the other files to be consistent I can change this after you merge.

@@ -120,7 +125,7 @@
*/
protected void constructWKT(SQLServerSpatialDatatype sd, InternalSpatialDatatype isd, int pointIndexEnd,
int figureIndexEnd, int segmentIndexEnd, int shapeIndexEnd) throws SQLServerException {
if (null == points || numberOfPoints == 0) {
if (null == xpoints || numberOfPoints == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

null=xpoints check is not needed. numberOfPoints is enough to be checked.

@@ -333,22 +339,20 @@ protected void parseWKTForSerialization(SQLServerSpatialDatatype sd, int startPo
*
*/
protected void constructPointWKT(int pointIndex) {
int firstPointIndex = pointIndex * 2;
int secondPointIndex = firstPointIndex + 1;
int zValueIndex = pointIndex;
int mValueIndex = pointIndex;
Copy link
Member

Choose a reason for hiding this comment

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

Please remove these unwanted mValueIndex and zValueIndex too.

@@ -38,7 +42,8 @@
protected int currentFigureIndex = 0;
protected int currentSegmentIndex = 0;
protected int currentShapeIndex = 0;
protected double points[];
protected double xpoints[];
protected double ypoints[];
protected double zValues[];
protected double mValues[];
Copy link
Member

@cheenamalhotra cheenamalhotra Jul 20, 2018

Choose a reason for hiding this comment

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

All these 4 arrays form contain point coordinates, naming should be consistent.

@@ -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

@@ -203,8 +216,8 @@ public boolean hasZ() {
* @return double value that represents the X coordinate.
*/
public Double getX() {
Copy link
Member

@cheenamalhotra cheenamalhotra Jul 20, 2018

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.

*
* @param x
* x coordinate
* @param y
* y coordinate
* @param srid
* SRID
* Spatial Reference Identifier Spatial Reference Identifier value
Copy link
Member

Choose a reason for hiding this comment

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

Twice?

@@ -215,9 +216,9 @@ 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

@@ -131,14 +131,15 @@ public static Geography parse(String wkt) throws SQLServerException {
}

/**
* Constructor for 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 a Spatial
* Reference Identifier.
*
* @param x
* x coordinate
* @param y
* y coordinate
Copy link
Member

Choose a reason for hiding this comment

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

Also fix params + javadocs for the constructor

rene-ye
rene-ye previously approved these changes Jul 23, 2018
@@ -403,10 +417,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.

@@ -2010,7 +2010,8 @@ public final void clearBatch() throws SQLServerException {
} 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.


private void checkNegSize(int num) throws SQLServerException {
if (num < 0) {
MessageFormat form = new MessageFormat(SQLServerException.getErrString("R_ParsingError"));
Copy link
Contributor

Choose a reason for hiding this comment

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

why don't you call throwIllegalWKB() instead here?

@ulvii
Copy link
Contributor

ulvii commented Jul 23, 2018

Approved, assuming we will improve the way we handle exceptions.

* @param srid
* SRID
* Spatial Reference Identifier value
* @return Geography Geography instance
* @throws SQLServerException
* if an exception occurs
Copy link
Member

Choose a reason for hiding this comment

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

I guess you missed out on the method params: x and y should be changed to latitude/longitude respectively.
Refer: MS Docs

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

@@ -22,17 +27,20 @@
* if an exception occurs
*/
private Geometry(String WellKnownText, int srid) throws SQLServerException {
Copy link
Member

Choose a reason for hiding this comment

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

First character in param name must not be capitalized. Please change to 'wellKnownText'

* 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. Spatial Reference Identifier 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

@@ -2010,7 +2010,8 @@ public final void clearBatch() throws SQLServerException {
} 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
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.

wkb = buf.array();

}
return;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this return statement here? Also a blank line in the else block above looks unnecessary.

@cheenamalhotra cheenamalhotra added this to the 7.0.0 milestone Jul 24, 2018
rene-ye
rene-ye previously approved these changes Jul 24, 2018
cheenamalhotra
cheenamalhotra previously approved these changes Jul 26, 2018
@peterbae peterbae changed the title Fuzz and javadoc fix Fuzz testing for spatial datatypes Jul 26, 2018
@@ -5,34 +5,38 @@

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?

@peterbae peterbae changed the title Fuzz testing for spatial datatypes API and JavaDoc changes for Spatial Datatypes Jul 26, 2018
coords[numOfCoordinates] = Double.NaN;
currentWktPos = currentWktPos + 4;
} else {
MessageFormat form = new MessageFormat(SQLServerException.getErrString("R_illegalWKTposition"));
Copy link
Contributor

@ulvii ulvii Jul 26, 2018

Choose a reason for hiding this comment

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

Please create a method throwIllegalWKTposition. There are at least 5 places where you construct the Exception.

@peterbae peterbae merged commit aa0f653 into microsoft:dev Jul 27, 2018
cheenamalhotra added a commit that referenced this pull request Jul 31, 2018
* Update Snapshot for upcoming RTW release v7.0.0

* Change order of logic for checking the condition for using Bulk Copy API (#736)

Fix | Change order of logic for checking the condition for using Bulk Copy API (#736)

* Update CHANGELOG.md

* Merge pull request #732 from cheenamalhotra/module (Export driver in automatic module)

Introduce Automatic Module Name in POM.

* Update CHANGELOG.md

* Change Sha1HashKey to CityHash128Key for generating PreparedStatement handle and metadata cache keys (#717)

* Change Sha1HashKey to CityHash128Key

* Formatted code

* Prepared statement performance fixes

1) Further speedups to prepared statement hashing

2) Caching of '?' chararacter positiobs in prepared statements to speed parameter substitution

* String compare for hash keys
added missing line for bulkcopy tests.

* comment change

* Move CityHash class to a separate file

* spacings fixes
cleaner code & logic

* Add | Adding useBulkCopyForBatchInsert property to Request Boundary methods (#739)

* Apply the collation name change to UTF8SupportTest

* Package changes for CityHash with license information (#740)

* Reformatted Code + Updated formatter (#742)

* Reformatted Code + Updated formatter

* Fix policheck issue with 'Country' keyword (#745)

* Adding a new test for beginRequest()/endRequest() (#746)

* Add | Adding a new test to notify the developers to consider beginRequest()/endRequest() when adding a new Connection API

* Fix | Fixes for issues reported by static analysis tools (SonarQube + Fortify) (#747)

* handle buffer reading

for invalid buffer input by user

* Revert "handle buffer reading"

This reverts commit 11e2bf4.

* updated javadocs (#754)

* fixed some typos in javadocs (#760)

* API and JavaDoc changes for Spatial Datatypes (#752)

Add | API and JavaDoc changes for Spatial Datatypes (#752)

* Disallow non-parameterized queries for Bulk Copy API for batch insert (#756)

fix | Disallow non-parameterized queries for Bulk Copy API for batch insert (#756)

* Formatting | Change scope of unwanted Public APIs + Code Format (#757)

* Fix unwanted Public APIs.
* Updated formatter to add new line to EOF + formatted project

* Release | Release 7.0 changelog and version update (#748)

* Updated Changelog + release version changes

* Changelog entry updated as per review.

* Updated changelog for new changes

* Terminology update: request boundary declaration APIs

* Trigger Appveyor

* Update Samples and add new samples for new features (#761)

* Update Samples and add new Samples for new features

* Update samples from Peter

* Updated JavaDocs

* Switch to block comment

* Update License copyright (#767)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants