Add correct lat, lng to parsed GEOGRAPHY POINT [#1277] #1282
Conversation
Since standard is part of the test script, it was failing before tests even ran. ``` standard: Use JavaScript Standard Style (https://standardjs.com) standard: Run `standard --fix` to automatically fix some problems. node-mssql/lib/tedious/request.js:271:44: Extra semicolon. node-mssql/lib/tedious/request.js:275:33: Extra semicolon. ```
Before, the test would timeout if an assertion tripped. Alongside that, Node would log an error about an unhandled rejection. Now, Mocha rapidly fails the test on the first failed assertion.
Before, this one test embedded an assumption that the tests would only run against the `master` database. Now, that assumption is removed, by reading the database name from the config file.
Since GEOGRAPHY POINT and GEOMETRY POINT are different, we need separate tests for these two UDTs. As 2.1 notes: > The term "GEOGRAPHY POINT structure" does not also refer to the GEOMETRY POINT structure in this document. ([MS-SSCLRT 2.1][]) Other than that fundamental unit of structure, they're identical. [MS-SSCLRT 2.1]: https://docs.microsoft.com/en-us/openspecs/sql_server_protocols/ms-ssclrt/8ce82728-6582-4e7b-96a0-8d0379767877
Before, it said false was not true. Now, it provides enough detail to understand why false was not true.
Before, it checked for instanceof when no message was provided. But it is never called without a message regexp, so this is unnecessary. (In case the unrunnable-test variation using the msnodesqlv8 driver ever gets un-skipped, I have adjusted the parameters to agree in providing a regexp, however.)
|
This is rather light on the integration tests. Should I also add an integration test that we can insert a point, parse it out, insert it per the parsed point info, and read it back again, with identical results? This would be easy to parameterize across geography vs geometry, since the WKT ordering of the axes is always x then y. Using values of x = 180 and y = 90, this would throw an error on insertion from the parsed struct if the axis ordering was flipped for the geography. I could also add a test to verify that the geography point accessors .Lat and .Long agree with our parsed x and y from the overall geography column. |
Regarding tests - I'm happy for you to cover off whatever you think is most appropriate :) |
dhensby
left a comment
dhensby
left a comment
There was a problem hiding this comment.
This looks really good, and thanks for cleaning up some of the quirks with the test suite, it could do with a good going-over to sort some of its quirks.
Just a few minor comments for you at this point :)
| describe('msnodesqlv8 connection errors', function () { | ||
| it('login failed', done => TESTS['login failed'](done, /Login failed for user '(.*)'\./)) | ||
| it.skip('timeout (not supported by msnodesqlv8)', done => TESTS.timeout(done, /1000ms/)) | ||
| it.skip('timeout (not supported by msnodesqlv8)', done => TESTS.timeout.call(this, done, /1000ms/)) |
There was a problem hiding this comment.
I think this will need to be wrapped in a non-arrow anon function as with the tedious.js file
There was a problem hiding this comment.
Pretty sure this still needs to be it('timeout', function (done) { TESTS.timeout.call(this, done, /Failed to connect to 10.0.0.1:1433 in 1000ms/) })
Section ref and details were correct, but "header" was wrong.
jeremy-w
changed the title
This makes this not-a-breaking-change, since it is purely additive.
Done by querying for the point's Lat and Long directly and comparing to our parsed fields.
jeremy-w
force-pushed
the
jeremy-w/fix/geography-point-coordinate-order-1277
branch
from
3ed1c00 to
1469ec9
Compare
|
@dhensby I believe I've addressed all the comments. I've updated the PR description to describe the backwards-compatible approach we decided on with adding correct lat/lng alongside unchanged (still flipped) x/y. And I extended the UDT integration test to confirm our notion of lat and lng agree with SQL Server's, as a final sanity check. |
dhensby
left a comment
dhensby
left a comment
There was a problem hiding this comment.
Great! Thanks so much for this work, really excellent contribution.
Only other thing I can think is the changelog and readme updates if there are any needed?
|
@dhensby i've updated the README. how do you handle adding changelog entries? in the past, i've accumulated changes in a Next Version and then updated the heading to record the version and release date as part of making a release. |
|
I tried to push a commit for the changelog, but I don't have permission to push to your branch. I'd just add this to the top of the changelog: |
|
@dhensby I've added that text to the changelog. I have "Allow edits by maintainers" checked, and that seems to include anyone who can push to the source of the fork, so I don't know why it didn't let you push. 🤔 |
2 participants
Parsing now differentiates between parsing a geometry vs parsing a geography. Everything about the two structures is identical, except for their POINT structs, which have different valid ranges and, most significantly for us, different axis orders:
However, in order to avoid breaking compatibility, this PR:
Users of the library are recommended to migrate to relying on the lat and lng fields when working with geographies, in preference to the x and y fields.