View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0020340 | mantisbt | attachments | public | 2015-12-02 21:47 | 2016-08-15 09:02 |
Reporter | lesley.liu | Assigned To | dregad | ||
Priority | high | Severity | major | Reproducibility | always |
Status | closed | Resolution | fixed | ||
Product Version | 1.2.16 | ||||
Target Version | 1.2.20 | Fixed in Version | 1.2.20 | ||
Summary | 0020340: Attachment is saved to disk and database at the same time | ||||
Description |
3a. If access /admin/move_attachments_page.php, the attachment count is not 0. query result the attachment: <pre> | ||||
Tags | No tags attached. | ||||
Not able to reproduce in current master (1.3.0-rc.1) |
|
You can't reproduce the issue but you confirm it, does this means it applies to 1.2.x ? |
|
The issues is reproducible in 1.2.19 but does no longer appear in current master. |
|
Ok, thanks for the clarification. I'll backport this commit then. |
|
I believe this is a regression that was introduced in 1.2.16 (commit 4f4e69bd7) when db_query() call was replaced by db_query_bound(). |
|
The fix is easy enough, but I'm wondering about the consequences of the regression, i.e. all the instances out there relying on DISK for attachments storage and potentially have a large number of invalid blobs in their mantis_bug_file_table. This can be addressed by a simple SQL statement updating all blobs containing "''" and changing that to ''. The "clean" way to handle this would be to write an install function, but I can't do this in 1.2.x as it would break the schema upgrade sequence. Any ideas ? |
|
I would add a cleanup upgrade function in 1.3.0 so that this is cleaned up as part of upgrading to 1.3.0. I'm also OK adding schema upgrade for 1.2.20, we can revisit our process there given how long it takes between 1.2.0 and 1.3.0 for example. |
|
Thinking about this more, we won't be able to add an upgrade function to 1.2.x since we identify upgrade steps by their sequence number now. So we can bundle the check / fix in the system utilities / check.php. We can potentially provide a warning to the administrator if they have blobs that needs cleanup when they are in the management area. I would like to see us move (in 1.3.x) to a model where we track upgrade steps by an id rather than a sequence number. That would enable us to update schema in two branches in parallel. For examples, "api-tokens-1", "api-tokens-2", rather than 201 and 202. |
|
In this special case we could add a step 184 in master-1.2.x as step 184 in master is |
|
Changing old upgrade steps typically ends up causing issues, so I would rather avoid it. How about if we apply a code fix for 1.2.x that would treat this corrupted content values as empty blobs? And then clean such blobs when users upgrade to 1.3.x and hence, we don't need to code fix that goes only on 1.2.x. |
|
I have one 1.3.0-beta3 DB which doesn't have this issue. The change in core/file_api.php should fix it. Is there a target date to provide the next stable release? |
|
Of course, changes like this one 0006626:0029192 (using an index in master-1.2.x that is used for another purpose in master) will introduce trouble for some installations. We could add in master-1.2.x This shouldn't introduce any issue as master will still be |
|
No, but there is a newer 1.3.0-rc.1 |
|
Thanks for your feedback.
Exactly, this is why I asked for ideas.
That would probably work actually, since it's not a blocking issue - users would only hit the problem when switching to DATABASE upload method or trying to move attachments to disk as the OP found out.
Well, I would argue that a sequence number is an ID... Joke aside, I'm not seeing that much benefit of switching from a numeric, sequential ID to a string. I find it useful to have an ID which both identifies the upgrade step as well as where it belongs in the sequence. Furthermore, it would become complicated to identify which steps have been applied, when, and in which sequence. That being said, maybe the ID doesn't need to be strictly sequential. In other words, as an alternative to your suggestion, we could "reserve" a block of upgrade steps when we switch to a new major version, allowing more flexibility for required schema changes within a minor release. For example, an hypothetical 2.0.0 schema.php could look like this: Release marker: 1.3.0upgrade steps 202..219 are reserved for 1.3.x releases$g_upgrade[220] = [...] Release marker: 2.0.0</pre> The problem with this approach is that it would need some special processing to cover the following scenario :
Adding this only at step 184 would hit the same scenario I described above. Considering your update in 0020340:0052037, adding the step again at 2020 would probably work in this specific case, because executing the upgrade function twice would not have any side effects. As a side note,
is really useless - the upgrade step could simply be set to NULL (since 1.3.0-beta.1 / 061e66b5).
If you installed it straight from >= 1.30.0-beta.1 then you should be fine |
|
@atrol @dregad |
|
Just to clarify, the important part in my earlier comment is not how you installed the code but rather which version of Mantis was used to install or upgrade DB prior to the 1.3.0-beta/rc upgrade. Anyway if you don't have any file attachments in the DB with just '' in the content field, then you're OK (this can be checked at DB level). |
|
Yes, there is no '' existed in content of mantis_bug_file_table of that 2 DBs. It was only happened on 1.2.19 in our server. |
|
Reminder sent to: dregad, vboctor I think this issue is a blocking one for 1.3. If we use upgrade step 184 (the one without any functionality at the moment) to cleanup the database we would have a fix for it in 1.2.20. |
|
My suggestion for end state is:
I also suggest that we either open an issue with target version 1.3.x that matches the blocking filter so that we don't lose track of this for 1.3.0 release. |
|
MantisBT 1.3.0 stable has been released. Not sure if there is still a need to worry about this for 1.2.x since users should upgrade to 1.3.0 stable. |
|
The issue is fixed in 1.3.0 in a fresh install. |
|
With 1.3.0 now live, I don't think it's necessary anymore to add an admin check in 1.2.x to detect the situation. Instead, we should focus on add a schema upgrade step (i.e. install function to repair corrupt attachments). Considering that this issue tracks the fix of the root cause, i.e. the generation of the invalid attachments when converting from DB to DISK, I will resolve it again, and reopen 0020540 for follow-up in 1.3.x. |
|
MantisBT: master-1.2.x e9260b14 2015-12-06 06:59 Details Diff |
Fix 2-byte attachment saved to DB when upload_method is DISK Commit 4f4e69bd73e2840ce4542072bb544f51ddaf3cf7 changed file_add() to use bind parameters, but failed to remove quotes escaping of $c_content variable. This results in an invalid 2-byte attachment (containing 2 single quotes "''") being stored in the file table in addition to the one saved to disk as expected, which does not actually have any impact unless the admin tries to move attachments from DB to disk; in that case, the valid files are overwritten, causing loss of data. The problem is fixed by setting $c_content to an empty string. Previously fixed in master branch commit 02bbb99501c48f89ad249fb505dc9bba93bae5c0 Fixes 0020340 |
Affected Issues 0020340 |
|
mod - core/file_api.php | Diff File |