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

[NFR]: Support POINT column type in MySQL #14769

Closed
jesugmz opened this issue Jan 24, 2020 · 10 comments
Closed

[NFR]: Support POINT column type in MySQL #14769

jesugmz opened this issue Jan 24, 2020 · 10 comments
Assignees
Labels
new feature request Planned Feature or New Feature Request

Comments

@jesugmz
Copy link

jesugmz commented Jan 24, 2020

Describe the bug
Having a MySQL column with type POINT the reading of its value is not well retrieved.

To Reproduce

Phalcon Micro application:

<?php

use Phalcon\Db\Adapter\Pdo\Mysql;
use Phalcon\Db\RawValue;
use Phalcon\Di\FactoryDefault;
use Phalcon\Http\Response;
use Phalcon\Mvc\Micro;
use Phalcon\Mvc\Model;

class Items extends Model
{
    /**
     * @var integer
     * @Primary
     * @Identity
     * @Column(type="integer", length=3, nullable=false)
     */
    public $id;

    /**
     * @var string
     * @Column(type="string", length=60, nullable=false)
     */
    public $location;

    public function initialize()
    {
        $this->setSchema('test');
        $this->setSource('items');
    }
}

$di = new FactoryDefault();
$di->setShared('db', function () {
    return new Mysql([
        'host'     => 'my-host',
        'username' => 'root',
        'password' => 'root',
        'dbname'   => 'test',
        'charset'  => 'utf8',
    ]);
});

$app = new Micro($di);

// broken
$app->get('/items/{id:[\d]}', function ($id) use ($app) {
  $item = Items::findFirst('id = '.$id);
  return $this->response->setJsonContent($item);
});

// works
$app->post('/items', function () use ($app) {
  $item = new Items();
  $item->location = new RawValue('ST_GeomFromText("POINT(1.0 1.0)")');
  $created = $item->save();

  return $created ? $this->response->setStatusCode(201) : $this->response->setStatusCode(500);
});

// works
$app->put('/items/{id:[\d]}', function ($id) use ($app) {
  $item = Items::findFirst('id = '.$id);
  $item->location = new RawValue('ST_GeomFromText("POINT(5.0 5.0)")');
  //$item->location = $item->location; // this will break as the the reading is broken
  $updated = $item->update();

  return $updated ? $this->response->setStatusCode(200) : $this->response->setStatusCode(500);
});

$app->notFound(function() {
    $response = new Response();
    return $response->setStatusCode(404);
});

$app->handle();

Database schema:

CREATE DATABASE test DEFAULT CHARACTER SET utf8 COLLATE utf8_general_ci;
USE test;

CREATE TABLE items (
  id MEDIUMINT(3) UNSIGNED NOT NULL AUTO_INCREMENT,
  location POINT NOT NULL,
  PRIMARY KEY (id),
  SPATIAL INDEX (location)
) ENGINE=InnoDB DEFAULT CHARSET=utf8;

POST and PUT (writing a POINT column) operations will works but GET (reading a POINT column) will return a response like this:

{"id":"3","location":"\u0000\u0000\u0000\u0000\u0001\u0001\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0014@\u0000\u0000\u0000\u0000\u0000\u0000\u0014@"}

In Phalcon 3.4.4 the reading works well but not the writing which cannot modify the POINT column.

Details

  • Phalcon version: 4.0.2
  • PHP Version: 7.4.2
  • Operating System: Debian 10.2
  • Installation type: PECL
  • Server: Nginx
  • MySQL: 5.7.21-20
@jesugmz jesugmz added bug A bug report status: unverified Unverified labels Jan 24, 2020
@ruudboon ruudboon added this to Current Sprint (Ends January 24th) in Phalcon Roadmap Jan 25, 2020
@ruudboon ruudboon added 4.0.3 and removed status: unverified Unverified labels Jan 25, 2020
@ruudboon
Copy link
Member

I was able to reproduce this locally.

@ruudboon ruudboon added the status: low Low label Jan 25, 2020
@ruudboon
Copy link
Member

The POINT datatype isn't in our Column definition so it defaults to varchar. Probably it should be handled differently. I was about that say this is a feature request but it looks like we had this in before. #1536
Not sure why this failing in 4 and was working in 3.4 don't see any changes related to this.

@ruudboon
Copy link
Member

ruudboon commented Jan 25, 2020

What would your expected outcome will look like @jesugmz ?
Not familiar with mysql spacial types but looks like this works.

unpack('x/x/x/x/corder/Ltype/dlat/dlon', $item->location)

@jesugmz
Copy link
Author

jesugmz commented Jan 25, 2020

It makes sense, POINT type is binary and works in that way so more than fine @ruudboon!

But still exists the issue when updating. Seems adding $this->useDynamicUpdate(true); in the initialize method of the model is ignored.

Note I have added a new field name:

DROP DATABASE test;
CREATE DATABASE test DEFAULT CHARACTER SET utf8 COLLATE utf8_general_ci;
USE test;

CREATE TABLE items (
  id MEDIUMINT(3) UNSIGNED NOT NULL AUTO_INCREMENT,
  name VARCHAR(50),
  location POINT NOT NULL,
  PRIMARY KEY (id),
  SPATIAL INDEX (location)
) ENGINE=InnoDB DEFAULT CHARSET=utf8;
<?php

use Phalcon\Db\Adapter\Pdo\Mysql;
use Phalcon\Db\RawValue;
use Phalcon\Di\FactoryDefault;
use Phalcon\Http\Response;
use Phalcon\Mvc\Micro;
use Phalcon\Mvc\Model;

class Items extends Model
{
    /**
     * @var integer
     * @Primary
     * @Identity
     * @Column(type="integer", length=3, nullable=false)
     */
    public $id;

    /**
     * @var string
     * @Column(type="string", length=50, nullable=true)
     */
    public $name;

    /**
     * @var string
     * @Column(type="string", length=60, nullable=false)
     */
    public $location;

    public function initialize()
    {
        $this->setSchema('test');
        $this->setSource('items');
        $this->useDynamicUpdate(true);
    }
}

$di = new FactoryDefault();
$di->setShared('db', function () {
    return new Mysql([
        'host'     => 'my-host',
        'username' => 'root',
        'password' => 'root',
        'dbname'   => 'test',
        'charset'  => 'utf8',
    ]);
});

$app = new Micro($di);

$app->get('/items/{id:[\d]}', function ($id) use ($app) {
  $item = Items::findFirst('id = '.$id);
  $coordinates = unpack('x/x/x/x/corder/Ltype/dlat/dlon', $item->location);
  return $this->response->setJsonContent($coordinates);
});

$app->post('/items', function () use ($app) {
  $item = new Items();
  $item->location = new RawValue('ST_GeomFromText("POINT(1.0 1.0)")');
  $created = $item->save();

  return $created ? $this->response->setStatusCode(201) : $this->response->setStatusCode(500);
});

$app->put('/items/{id:[\d]}', function ($id) use ($app) {
  $item = Items::findFirst('id = '.$id);
  $item->name = 'test';
  $updated = $item->update();

  // will return 500 as seems to be doing internally $item->location = $item->location
  return $updated ? $this->response->setStatusCode(200) : $this->response->setStatusCode(500);
});

$app->notFound(function() {
    $response = new Response();
    return $response->setStatusCode(404);
});

$app->handle($_SERVER["REQUEST_URI"]);

but doing this, works:

$app->put('/items/{id:[\d]}', function ($id) use ($app) {
  $item = Items::findFirst('id = '.$id);
  $item->location = new RawValue($item->location);
  $item->name = 'test';
  $updated = $item->update();

  return $updated ? $this->response->setStatusCode(200) : $this->response->setStatusCode(500);
});

The problem with this is I have to do the POINT update manually even when I want just to update another field. As a patch works for me so far.

@ruudboon
Copy link
Member

I'm gonna file this one as a NFR. The type column isn't in our known DB columns yet. Also need to check if the POINT type is available on other databases as well.
Need to check if we can add this to 4.0.x or that we need to push this to 4.1. Any ideas @niden ?

@ruudboon ruudboon changed the title [BUG]: MySQL POINT broken when retrieving [NFR]: Support POINT column type in MySQL Jan 25, 2020
@ruudboon ruudboon moved this from Current Sprint (Ends January 24th) to Backlog in Phalcon Roadmap Jan 25, 2020
@ruudboon ruudboon added new feature request Planned Feature or New Feature Request and removed bug A bug report status: low Low labels Jan 25, 2020
@ruudboon ruudboon self-assigned this Jan 25, 2020
@jesugmz
Copy link
Author

jesugmz commented Jan 25, 2020

Apologizes for the mess guys. Just have tested again the case and works fine in 4.0.2 so even $item->location = $item->location; works well. In the last test (updating case) was testing 3.4.4.

I let the issue open in case you want to keep going with POINT type support.

Thanks!

@niden
Copy link
Sponsor Member

niden commented Jan 25, 2020

@jesugmz Thanks for reporting this.

As Ruud pointed out we pretty much never supported POINT - it was never in the column map. So that fact that it worked before was a side effect if you like.

There have been a lot of types added for v4 in our ORM but clearly we have not caught everything - especially taking into account the differences in RDBMs syntax.

I can keep this as a NFR but I propose we close it because we are working/will release a brand new ORM which will be more flexible for pretty much every RDBMS we support. That will solve this and related issues.

@ruudboon
Copy link
Member

@niden I see so use for this and don't think it's a lot of work to add the correct type. Can we close it and add it to the normal voting list?
@jesugmz thnx for the update

@niden
Copy link
Sponsor Member

niden commented Jan 25, 2020

@ruudboon Perfectly fine with that.

@ruudboon
Copy link
Member

Closing this in favour of #14608 (comment)

Phalcon Roadmap automation moved this from Backlog to Implemented Jan 25, 2020
@niden niden moved this from Implemented to Released in Phalcon Roadmap Jan 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature request Planned Feature or New Feature Request
Projects
Archived in project
Phalcon Roadmap
  
Released
Development

No branches or pull requests

3 participants