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

fix: properly select SRID if present #11763

Merged
merged 29 commits into from Jan 21, 2020
Merged

fix: properly select SRID if present #11763

merged 29 commits into from Jan 21, 2020

Conversation

MuhammedKalkan
Copy link
Contributor

@MuhammedKalkan MuhammedKalkan commented Dec 23, 2019

Fix SRID problem. Now short SRID is selected if present.

Pull Request check-list

Please make sure to review and check all of these items:

  • Does npm run test or npm run test-DIALECT pass with this change (including linting)?
  • Does the description below contain a link to an existing issue (Closes #[issue]) or a description of the issue you are solving?
  • Have you added new tests to prevent regressions?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Did you update the typescript typings accordingly (if applicable)?
  • Did you follow the commit message conventions explained in CONTRIBUTING.md?

Description of change

Add short SRID selection as default if present at the database cell.
Closes #11615

Fix SRID problem. Now short SRID is selected if present.
@codecov
Copy link

@codecov codecov bot commented Dec 23, 2019

Codecov Report

Merging #11763 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #11763   +/-   ##
=======================================
  Coverage   96.27%   96.27%           
=======================================
  Files          94       94           
  Lines        9197     9197           
=======================================
  Hits         8854     8854           
  Misses        343      343
Impacted Files Coverage Δ
lib/dialects/postgres/connection-manager.js 95.77% <ø> (ø) ⬆️

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 2382d8b...e9aa99b. Read the comment docs.

@papb
Copy link
Member

@papb papb commented Dec 23, 2019

Hello! I see you are a first-time contributor, thank you for taking the time to help Sequelize! I hope to see more PRs from you in the future!

Please add at least one test to make sure your implementation works as intended and to prevent regressions in the future. Take a look into recent merged PRs to see how others created tests. If you have trouble, let me know.

@papb papb added dialect: postgres status: awaiting response labels Dec 23, 2019
@MuhammedKalkan
Copy link
Contributor Author

@MuhammedKalkan MuhammedKalkan commented Dec 24, 2019

Updated tests, but somehow irrelevant sqlite tests are failing. Care to inspect ?

@papb

@papb papb changed the title Update data-types.js fix(postgres): properly select SRID if present Jan 16, 2020
@papb
Copy link
Member

@papb papb commented Jan 16, 2020

Hi @Nymria I see that you are working on top of my modifications, great 😬 hopefully you will manage to get everything green!

By the way, you can ignore errors of the form Error: Expected 0 running queries. 1 queries still running [...]

image

@papb
Copy link
Member

@papb papb commented Jan 16, 2020

Let me know when the PR is ready for review again!

@papb papb added the type: bug label Jan 16, 2020
@MuhammedKalkan
Copy link
Contributor Author

@MuhammedKalkan MuhammedKalkan commented Jan 16, 2020

Well seems everything is in order. Only postgresql test are failing the way you described

Error: Expected 0 running queries. 1 queries still running [...]

@papb
Copy link
Member

@papb papb commented Jan 16, 2020

Hi @Nymria, looks good overall! I have one question: it seems that you added a crs field on every test that could take it, does this mean that providing a crs has gotten necessary now with your PR?

@MuhammedKalkan
Copy link
Contributor Author

@MuhammedKalkan MuhammedKalkan commented Jan 16, 2020

Not necessary optional. if crs field is provided you should receive back. want me to revert other test cases ? Maybe that is more convenient

@papb
Copy link
Member

@papb papb commented Jan 16, 2020

Yes, please revert the other test cases then. If you think it's worth testing crs on them as well, add new tests instead, thanks!

test/integration/dialects/postgres/data-types.test.js Outdated Show resolved Hide resolved
test/integration/dialects/postgres/data-types.test.js Outdated Show resolved Hide resolved
@MuhammedKalkan
Copy link
Contributor Author

@MuhammedKalkan MuhammedKalkan commented Jan 17, 2020

Alright, i wasnt using GEOGRAPHY type and interested in GEOMETRY type. For geometry type SRID can be optional, but as much as i understand GEOGRAPHY type will have an SRID field by default.

Here it says it will use EPSG:4326 if none is pointed out.

https://postgis.net/docs/using_postgis_dbmanagement.html#PostGIS_Geography

If you do not specify an SRID, the SRID will default to 4326 WGS 84 long/lat will be used, and all calculations will proceed using WGS84.

So i will add CRS field test for whole geography types.

If you prefer, i can remove CRS retrieval from DB for GEOGRAPHY type aswell.

@papb
Copy link
Member

@papb papb commented Jan 17, 2020

Hi @Nymria, thanks, I am a bit confused though, can you explain again why you added crs back to all geography tests? Didn't they also pass without that?

@MuhammedKalkan
Copy link
Contributor Author

@MuhammedKalkan MuhammedKalkan commented Jan 17, 2020

@papb Yeah , it is not like geometry type, geography type fails without them. Since srid is always defined by postgis for the geography type, query returns crs value weather it is given by user or not. Therefore test input goes in without crs and comes out with default crs , which seems to be the behaviour of postgis. previous comment contains postgis link regarding subject.

Copy link
Contributor

@sushantdhiman sushantdhiman left a comment

Please enable this for MySQL & MariaDB

Muhammed Kalkan added 2 commits Jan 19, 2020
add short crs selection for mysql and mariadb
remove tests  to geometry model
@MuhammedKalkan MuhammedKalkan requested a review from papb Jan 19, 2020
@MuhammedKalkan MuhammedKalkan changed the title fix(postgres): properly select SRID if present fix: properly select SRID if present Jan 19, 2020
@MuhammedKalkan MuhammedKalkan requested a review from papb Jan 20, 2020
papb
papb approved these changes Jan 21, 2020
@papb
Copy link
Member

@papb papb commented Jan 21, 2020

LGTM, however I would like to wait for a final review by @sushantdhiman before merging.

@papb papb added status: awaiting maintainer and removed status: awaiting response labels Jan 21, 2020
@sushantdhiman sushantdhiman merged commit cb22281 into sequelize:master Jan 21, 2020
4 checks passed
@papb papb removed the status: awaiting maintainer label Jan 21, 2020
@yocontra
Copy link
Contributor

@yocontra yocontra commented Aug 24, 2020

IMO I think adding:

if (o.crs && o.crs.properties.name === 'EPSG:4326') delete o.crs; is appropriate for all of the parsers here . The crs attribute is deprecated in GeoJSON and all GeoJSON is to be assumed to be WGS84 - including the projection where it isn't needed is flying against the spec + (not like this is a huge issue) consuming more memory/bandwidth.

I would be happy to send a PR to amend this and not include the crs property when not needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dialect: postgres type: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants