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

Add missing migrations for longtext and indices #654

Merged
merged 1 commit into from
Oct 24, 2018

Conversation

PVince81
Copy link
Contributor

@PVince81 PVince81 commented Oct 19, 2018

  • set type to "longtext" for "subjectparams" and "messageparams" by increasing length
  • add index "activity_time" on column "timestamp"
  • add index "activity_object" on columns "object_type" and "object_id"

Fixes #652

@PVince81
Copy link
Contributor Author

Core PR to implement the same code from upstream: owncloud/core#33231.
This helps fix the issue but we can't automatically test this due to DBAL limitations.

@PVince81
Copy link
Contributor Author

I guess it won't be possible to make Oracle comply without gymnastics similar to https://github.com/owncloud/activity/blob/master/appinfo/Migrations/Version20170131134507.php#L18

@DeepDiver1975
Copy link
Member

Core PR to implement the same code from upstream: owncloud/core#33231.
This helps fix the issue but we can't automatically test this due to DBAL limitations.

for the oracle part we could leave a release note and tell people what to do ... there are only a few installations .....

1 similar comment
@DeepDiver1975
Copy link
Member

Core PR to implement the same code from upstream: owncloud/core#33231.
This helps fix the issue but we can't automatically test this due to DBAL limitations.

for the oracle part we could leave a release note and tell people what to do ... there are only a few installations .....

@PVince81
Copy link
Contributor Author

@DeepDiver1975 in my local tests with core I noticed that Oracle would anyway convert the column to CLOB when asking for longtext. It looks like https://github.com/owncloud/activity/blob/master/appinfo/Migrations/Version20170131134507.php#L18 is already doing that. This means that we don't need to care about Oracle in activity for the longtext columns.

@PVince81
Copy link
Contributor Author

had a chat with @DeepDiver1975 and due to the missing DBAL feature and also the fact that this kind of migration is seldom, we'll write a migration containing direct SQL statements instead, which removes the need for the core workaround PR.

@PVince81
Copy link
Contributor Author

Seems PostgreSQL has no distinction between longtext and text, it's all "text" so we likely also do not need any change.

@PVince81
Copy link
Contributor Author

updated with plain SQL statement for MySQL.

I'll have a test with PostgreSQL just to make sure there aren't other discrepancies and to confirm that no SQL change is needed.

@PVince81
Copy link
Contributor Author

I've checked with PostgreSQL and apparently the column is already "text" and has no limit:

                                                                Table "public.oc_activity"
    Column     |          Type           | Collation | Nullable |                     Default                      | Storage  | Stats target | Des
cription 
---------------+-------------------------+-----------+----------+--------------------------------------------------+----------+--------------+----
---------
 activity_id   | bigint                  |           | not null | nextval('oc_activity_activity_id_seq'::regclass) | plain    |              | 
 timestamp     | integer                 |           | not null | 0                                                | plain    |              | 
 priority      | integer                 |           | not null | 0                                                | plain    |              | 
 type          | character varying(255)  |           |          | NULL::character varying                          | extended |              | 
 user          | character varying(64)   |           |          | NULL::character varying                          | extended |              | 
 affecteduser  | character varying(64)   |           | not null |                                                  | extended |              | 
 app           | character varying(255)  |           | not null |                                                  | extended |              | 
 subject       | character varying(255)  |           | not null |                                                  | extended |              | 
 subjectparams | text                    |           | not null |                                                  | extended |              | 
 message       | character varying(255)  |           |          | NULL::character varying                          | extended |              | 
 messageparams | text                    |           |          |                                                  | extended |              | 
 file          | character varying(4000) |           |          | NULL::character varying                          | extended |              | 
 link          | character varying(4000) |           |          | NULL::character varying                          | extended |              | 
 object_type   | character varying(255)  |           |          | NULL::character varying                          | extended |              | 
 object_id     | bigint                  |           | not null | 0                                                | plain    |              | 

so we only need to handle MySQL as already in the current PR code.

@codecov-io
Copy link

codecov-io commented Oct 22, 2018

Codecov Report

Merging #654 into master will decrease coverage by 1.09%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff             @@
##             master     #654     +/-   ##
===========================================
- Coverage     86.57%   85.47%   -1.1%     
- Complexity      495      500      +5     
===========================================
  Files            36       38      +2     
  Lines          1788     1811     +23     
===========================================
  Hits           1548     1548             
- Misses          240      263     +23
Impacted Files Coverage Δ Complexity Δ
appinfo/Migrations/Version20181022150134.php 0% <0%> (ø) 2 <2> (?)
appinfo/Migrations/Version20181019151118.php 0% <0%> (ø) 3 <3> (?)

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 1dac855...6aafcbe. Read the comment docs.

/**
* @author Vincent Petry <pvince81@owncloud.com>
*
* @copyright Copyright (c) 2018, ownCloud, Inc.
Copy link
Member

Choose a reason for hiding this comment

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

GmbH

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copy-pasted this file from another migration, so it's wrong there as well.

I'll adjust them all then

/**
* @author Vincent Petry <pvince81@owncloud.com>
*
* @copyright Copyright (c) 2018, ownCloud, Inc.
Copy link
Member

Choose a reason for hiding this comment

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

GmbH

- set type to "longtext" for "subjectparams" and "messageparams" by increasing length
- add index "activity_time" on column "timestamp"
- add index "activity_object" on columns "object_type" and "object_id"
@PVince81
Copy link
Contributor Author

fixed license row

@DeepDiver1975 DeepDiver1975 merged commit 36b0170 into master Oct 24, 2018
@DeepDiver1975 DeepDiver1975 deleted the add-missing-migrations branch October 24, 2018 13:04
@PVince81 PVince81 modified the milestones: development, QA Nov 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants