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

Array field in PostgreSQL return ArrayExpression instead of array #15716

Closed
tQuant opened this issue Feb 21, 2018 · 13 comments · Fixed by #15733
Closed

Array field in PostgreSQL return ArrayExpression instead of array #15716

tQuant opened this issue Feb 21, 2018 · 13 comments · Fixed by #15733

Comments

@tQuant
Copy link

tQuant commented Feb 21, 2018

There is no point to return ArrayExpression for array fields when loading ActiveRecord from database.
ArrayExpression do not have any usefull information per value. Data type and dimension can be obtained from the table scheme.

What steps will reproduce the problem?

Create an array postgresql field. Create ActiveRecord model. Load model from database.

What is the expected result?

Field contain array

What do you get instead?

Field contain ArrayExpression

Additional info

Q A
Yii version 2.0.14
PHP version 5.6
Operating system Debian 9
@SilverFire SilverFire self-assigned this Feb 21, 2018
@SilverFire SilverFire added this to the 2.0.14.1 milestone Feb 21, 2018
@SilverFire
Copy link
Member

Thank you for opening this issue.

On the first iterations of Arrays support, the AR model's fields got arrays after data population. From some point of discussions, we've decided to create ArrayExpression object after data population to keep things more consistent, but I don't remember the exact use cases that pushed me to change the implementation. I'm not really sticked to the current one, so we can discuss and change it, if it turns out it does not make much sense.

We already have two unexpected behaviors related to this decision. See #15710

@yiisoft/core-developers

@ekzobrain
Copy link

ekzobrain commented Feb 21, 2018

Yes, I already discovered two issues with this approach. And I think there might be much more.
But as i remember, one of the reasons of choosing such approach was backward incompatibility with query builder and type casting bacause DB query methods accept field values as an array when explicit type casting is required...

@klimov-paul
Copy link
Member

From some point of discussions, we've decided to create ArrayExpression object after data population to keep things more consistent, but I don't remember the exact use cases that pushed me to change the implementation.

This is the matter of lazy loading.
When fetching row with ARRAY or JSON from the database it is unknown whether those fields will be in use anywhere. Thus it is better to leave parsing for the later stage - to the moment it will be in need first time.

Having large JSON or ARRAY data may provide significant impact while being parsing over hunderds of records fetched from DB.

Also not every complex DB type can be converted to an array. For example, while using PostGis extension, its specific type 'geometry' can not be represented as mere array - it should be a specific complex object.

Still, this matter is debatable.

@zhdanovartur
Copy link
Contributor

Great feature, but I think ArrayExpression is not a PATCH update, because it broke a lot of model getters, used before for Postgres array fields.

@SilverFire
Copy link
Member

SilverFire commented Feb 21, 2018

It was not a PATCH update indeed. We have Semver shifted on one group to the right: 2.<major>.<minor>.<patch>, so 2.0.14 is a minor update. However, ArrayExpression still breaks compatibility and it was not clearly stated in UPGRADE.md.

@zhdanovartur what is the best solution on your opinion?

We are thinking about adding a flag for Schema class, that allows to return array instead of ArrayExpression, or to return string as it was used before.

@tQuant
Copy link
Author

tQuant commented Feb 21, 2018

When fetching row with ARRAY or JSON from the database it is unknown whether those fields will be in use anywhere. Thus it is better to leave parsing for the later stage - to the moment it will be in need first time.

It would make sense. But for now it parsed on phpTypecast. So it always parsing, no matter need or not.

If we talk about consistency, then the json fields are now returned as an array, not JsonExpresson, but array fileds returned as ArrayExpression. It looks strange to me.

We are thinking about adding a flag for Schema class, that allows to return array instead of ArrayExpression, or to return string as it was used before.

Flag is looking good to me. Unexpected ArrayExpression broke some code in my projects.
And if it were just arrays instead, then I would not have noticed anything

@SilverFire
Copy link
Member

It would make sense. But for now it parsed on phpTypecast. So it always parsing, no matter need or not.

Laziness can be easily added afterwards and nobody will notice the change ;)

If we talk about consistency, then the json fields are now returned as an array, not JsonExpresson, but array fileds returned as ArrayExpression. It looks strange to me.

But yes, that does not look very consistent, so it is a point of discussion now.

Unexpected ArrayExpression broke some code in my projects.

Could you check, whether it is any better with fix in #15733?

@zhdanovartur
Copy link
Contributor

zhdanovartur commented Feb 22, 2018

We are thinking about adding a flag for Schema class

Good idea.

@tQuant
Copy link
Author

tQuant commented Feb 22, 2018

Could you check, whether it is any better with fix in #15733?

It not work for in_array
in_array() expects parameter 2 to be array, object given

It's not hard for me to fix my code, but for backwards compatibility (if needed), I think the flag is the only option

@SilverFire
Copy link
Member

SilverFire commented Feb 23, 2018

Added flag in #15755

Can be configured somehow like this:

'db' => [
    'class' => \yii\db\Connection::class,
    'schemaMap' => [
        'pgsql' => [
            'class' => \yii\db\pgsql\Schema::class,
            'columnSchemaClass' => [
                'class' => \yii\db\pgsql\ColumnSchema::class,
                'disableJsonSupport' => true,
                'disableArraySupport' => true,
                'deserializeArrayColumnToArrayExpression' => false,
            ],
        ],
    ],
],

Or easier:

[
    'container' => [
        'definitions' => [
            \yii\db\pgsql\ColumnSchema::class => [
                'class' => \yii\db\pgsql\ColumnSchema::class,
                'disableJsonSupport' => true,
            ],
        ],
    ],
]

@tQuant Could you check with code from new PR and an appropriate config?

@SilverFire
Copy link
Member

Fixed by 25d176a

@tvxcz
Copy link

tvxcz commented Jul 29, 2019

Does anybody tested this fix in FULL reality?
For me, sample not work.
Config is setup accordingly, both methods, as I see it, even Class \yii\db\Connection receives it, but ColumnSchema then receives just empty configuration.
I'm testing with Yii2 version 2.0.23 and postgres.

@tvxcz
Copy link

tvxcz commented Nov 10, 2020

We've spent days to fix our system and about day to figure out why this approach described here don't work year and half ago. Without result.

We neded to move on now so my colegue now tried this again... after long debug of how all this work we finally found out basic reason why it won't work...
Problem was CACHE! This setting is THE ONLY ONE, that I know now that is cached in yii cache! Working with it for years I never had problem with some config directive to not work because of being cached until this one.

So, if this does'nt work, flush the cache to change values and reread config.

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

Successfully merging a pull request may close this issue.

7 participants