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

support for POINT class #877

Closed
wants to merge 2 commits into from
Closed

support for POINT class #877

wants to merge 2 commits into from

Conversation

kevinhikaruevans
Copy link

Since node-mysql supports SELECTing and casting the POINT class, it would make sense to also support escaping POINTs during INSERTs and UPDATEs. Currently, if you SELECT a field with a point, it'll return a plain object with x and y attributes. This PR will allow it to also INSERT and UPDATE an object with x and y attributes.

I have no idea if this was the correct way to append a new type in SqlString, but it works fine for me. Date casts were defined there, so it seemed like a good place to start. The POINT function is defined here.

@dougwilson
Copy link
Member

It needs tests, if you are so inclined :)

@kevinhikaruevans
Copy link
Author

Added a test for it! Not sure how many tests are required. There probably should be a separate function for geometry in SqlString, like SqlString.geometryToFunction() or something.

@dougwilson
Copy link
Member

That's fine, I just wanted a test so I know what you were expecting to happen. So I see form the test, it has the issue I described, where before your change that objects would have resulted in x = 0, y = 1, but now it results in POINT(0,1) which is totally breaking. I thought you said you would make it only apply to objects within other objects passed to escape?

For example, this use-case needs to continue to work with your change:

connection.query('INSERT INTO data SET ?', {x: 3, y: 7})

where there is an x column and a y column on the data table.

@kevinhikaruevans
Copy link
Author

Oh, I intended it for it to be objects within objects. I didn't know escape would be called on all objects being passed, I just thought it was called on objects being passed into single fields.

@dougwilson
Copy link
Member

OK, as long as that is your intention. I can adjust your PR then later :)

@@ -36,6 +36,10 @@ SqlString.escape = function(val, stringifyObjects, timeZone) {
}

if (typeof val === 'object') {
// for the mysql point class
if(val.hasOwnProperty('x') && val.hasOwnProperty('y')) {
Copy link
Member

Choose a reason for hiding this comment

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

We can also make this a little less risky by saying "if it has x and y and no other properties"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

2 participants