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

6 participants
@peterbae
Member

peterbae commented Jul 19, 2018

Changes needed after fuzz testing.

@codecov-io

This comment has been minimized.

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.

This comment has been minimized.

@lilgreenbird

lilgreenbird Jul 19, 2018

Member

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 {

This comment has been minimized.

@lilgreenbird

lilgreenbird Jul 19, 2018

Member

needs class description

@peterbae peterbae requested review from rene-ye, cheenamalhotra and ulvii Jul 19, 2018

@@ -198,7 +211,7 @@ public boolean hasZ() {
}
/**
* Returns the X coordinate value.
* Gets the X coordinate value.

This comment has been minimized.

@lilgreenbird

lilgreenbird Jul 19, 2018

Member

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) {

This comment has been minimized.

@cheenamalhotra

cheenamalhotra Jul 20, 2018

Member

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;

This comment has been minimized.

@cheenamalhotra

cheenamalhotra Jul 20, 2018

Member

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[];

This comment has been minimized.

@cheenamalhotra

cheenamalhotra Jul 20, 2018

Member

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.

This comment has been minimized.

@cheenamalhotra

cheenamalhotra Jul 20, 2018

Member

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() {

This comment has been minimized.

@cheenamalhotra

cheenamalhotra Jul 20, 2018

Member

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

This comment has been minimized.

@cheenamalhotra
@@ -215,9 +216,9 @@ public boolean hasZ() {
*
* @return double value that represents the X coordinate.

This comment has been minimized.

@cheenamalhotra

cheenamalhotra Jul 23, 2018

Member

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

This comment has been minimized.

@cheenamalhotra

cheenamalhotra Jul 23, 2018

Member

Also fix params + javadocs for the constructor

peterbae added some commits Jul 23, 2018

@@ -403,10 +417,11 @@ protected void parseWkb() {
}
private void readPoints() {

This comment has been minimized.

@ulvii

ulvii Jul 23, 2018

Member

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) {

This comment has been minimized.

@ulvii

ulvii Jul 23, 2018

Member

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

This comment has been minimized.

@peterbae

This comment has been minimized.

@cheenamalhotra

cheenamalhotra Jul 24, 2018

Member

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"));

This comment has been minimized.

@ulvii

ulvii Jul 23, 2018

Member

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

@ulvii

This comment has been minimized.

Member

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

This comment has been minimized.

@cheenamalhotra

cheenamalhotra Jul 24, 2018

Member

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

This comment has been minimized.

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

This comment has been minimized.

@cheenamalhotra

cheenamalhotra Jul 24, 2018

Member

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

This comment has been minimized.

@cheenamalhotra

cheenamalhotra Jul 24, 2018

Member

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

This comment has been minimized.

@peterbae
@@ -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) {

This comment has been minimized.

@cheenamalhotra

cheenamalhotra Jul 24, 2018

Member

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;

This comment has been minimized.

@cheenamalhotra

cheenamalhotra Jul 24, 2018

Member

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

peterbae added some commits Jul 24, 2018

peterbae added some commits Jul 25, 2018

@peterbae peterbae changed the title from Fuzz and javadoc fix to Fuzz testing for spatial datatypes Jul 26, 2018

@@ -5,34 +5,38 @@
package com.microsoft.sqlserver.jdbc;
import java.nio.BufferUnderflowException;

This comment has been minimized.

@ulvii

ulvii Jul 26, 2018

Member

Do we still need to import this?

@peterbae peterbae changed the title from Fuzz testing for spatial datatypes to 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"));

This comment has been minimized.

@ulvii

ulvii Jul 26, 2018

Member

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

@ulvii

ulvii approved these changes Jul 26, 2018

@cheenamalhotra cheenamalhotra added this to Under Peer Review in MSSQL JDBC Jul 27, 2018

@peterbae peterbae merged commit aa0f653 into Microsoft:dev Jul 27, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla All CLA requirements met.
Details

MSSQL JDBC automation moved this from Under Peer Review to Closed/Merged PRs Jul 27, 2018

cheenamalhotra added a commit that referenced this pull request Jul 31, 2018

Update master post RTW 7.0 release (#766)
* 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