Edgewall Software
Modify

Opened 9 years ago

Closed 3 years ago

Last modified 14 months ago

#9612 closed enhancement (fixed)

`WikiPage` doesn't need the `ipnr` attribute

Reported by: Martin Scharrer <martin@…> Owned by: Ryan J Ollos
Priority: low Milestone: 1.3.1
Component: wiki system Version: 0.12-stable
Severity: minor Keywords: WikiPage model ipnr
Cc: Thijs Triemstra Branch:
Release Notes:

Remove ipnr from attachment and wiki models.

API Changes:

Added upgrade_tables method to DatabaseManager class. Added method to Table class for removing columns: remove_columns.

Description

Just saw this while changing th:WatchlistPlugin from own SQL code to the trac.wiki.model.WikiPage class:

Instances of the WikiPage class do not provide the ipnr as attribute as for all other wiki page values (version, author, etc.). In fact the _fetch method doesn't read this value from the DB.

If this is not by intention this should be fixed for consistency. Some plug-ins probably like to read this information out, e.g. I would have given the users the option to show it in their watchlist.

I would be willing to provide a patch.

Attachments (1)

t9612-no-common-columns.diff (2.9 KB ) - added by Jun Omae 3 years ago.

Download all attachments as: .zip

Change History (60)

comment:1 by Christian Boos, 9 years ago

Well, why do you need the ipnr? I always thought nobody was actually interested in that and thought that we should even remove it in later releases.

comment:2 by Remy Blank, 9 years ago

We removed the display of that information some time ago in 0.11-stable IIRC.

in reply to:  1 comment:3 by Martin Scharrer <martin@…>, 9 years ago

Replying to cboos:

Well, why do you need the ipnr? I always thought nobody was actually interested in that and thought that we should even remove it in later releases.

As I said, I'm writing a plugin which shows the wiki data to the user as a table and I just wanted to be as comprehensive as possible. If you think about this that way I will just drop it. It's not an important thing for the plugin.

comment:4 by Christian Boos, 9 years ago

Milestone: next-major-0.1X
Summary: `WikiPage` instances lack the `ipnr` attribute`WikiPage` doesn't need the `ipnr` attribute

All clear then. Let's remember to remove the ipnr information from the wiki table, at some later point.

in reply to:  4 comment:5 by Thijs Triemstra, 8 years ago

Cc: Thijs Triemstra added

Replying to cboos:

All clear then. Let's remember to remove the ipnr information from the wiki table, at some later point.

What version do you have in mind?

comment:6 by Christian Boos, 8 years ago

0.14 of course. I don't think we should do any more schema change at this point for 0.13, as we should slowly try to finalize this version.

comment:7 by Thijs Triemstra, 8 years ago

Milestone: next-major-0.1X0.14

comment:8 by Ryan J Ollos, 5 years ago

I was thinking we should add some notices to the documentation about the pending removal: log:rjollos.git:t9612-schedule-ipnr-removal.

comment:9 by Ryan J Ollos, 5 years ago

Changes from comment:8 committed to 1.0-stable in [13512], merged to trunk in [13513].

comment:10 by Ryan J Ollos, 5 years ago

Should we also remove ipnr from the Attachment class?

in reply to:  10 comment:11 by Jun Omae, 5 years ago

Should we also remove ipnr from the Attachment class?

Yeah. We should consistently remove attachment.ipnr if wiki.ipnr will be removed.

comment:12 by Ryan J Ollos, 5 years ago

Milestone: next-dev-1.1.xnext-major-releases

Thanks. I'll add notices to attachment module about deprecation of ipnr and pending removal, as in comment:8. Changing milestone since work will be done in milestone:1.3.1.

comment:13 by Jun Omae, 5 years ago

Related question, should we also remove [trac] show_ip_addresses option in 1.3.1?

comment:14 by Ryan J Ollos, 5 years ago

Yeah, it looks like that should be removed. The only uses I see aside from adding the value to the Chrome data dictionary are:

Side note: I didn't realize it was possible to link to a specific option and highlight it. I guess that works with any wiki word or phrase though. Very cool!

comment:15 by Ryan J Ollos, 5 years ago

Deprecated ipnr attribute of attachment class in [13729:13730]. Deprecated show_ip_addresses in [13731:13732].

comment:16 by Ryan J Ollos, 4 years ago

Milestone: next-major-releases1.3.1

comment:17 by carstenklein@…, 3 years ago

What about a hacked trac and where people upload unwanted attachments or alter wiki pages in ways we cannot imagine? Wouldn't it be nice to at least have the ip address from where the alteration or malicious upload came from?

comment:18 by Ryan J Ollos, 3 years ago

For a publicly-accessible site, you probably want to be running SpamFilter. See also #12398.

Version 0, edited 3 years ago by Ryan J Ollos (next)

comment:19 by Ryan J Ollos, 3 years ago

Owner: set to Ryan J Ollos
Status: newassigned

comment:20 by Ryan J Ollos, 3 years ago

Proposed changes in log:rjollos.git:t9612.

comment:21 by Christian Boos, 3 years ago

LGTM, + 2 remarks:

  • we should also remove show_ip_addresses in chrome.py
  • db42.py makes me think we could start to introduce a DDL helper functions, like upgrade_table(old_Table, new_Table) and a Table.remove_columns that would create a new Table instance, so that one could write upgrade_table(attachment_table.remove_columns('ipnr'), attachment_table)

in reply to:  21 comment:22 by Ryan J Ollos, 3 years ago

Replying to Christian Boos:

  • we should also remove show_ip_addresses in chrome.py

show_ip_addresses was removed in r14909. That change should probably have been committed with these changes.

  • db42.py makes me think we could start to introduce a DDL helper functions, like upgrade_table(old_Table, new_Table) and a Table.remove_columns that would create a new Table instance, so that one could write upgrade_table(attachment_table.remove_columns('ipnr'), attachment_table)

Similar aim in #11872. I'll look into it.

comment:23 by Ryan J Ollos, 3 years ago

Proposed changes in log:rjollos.git:t9612.2. Some of the changes will be committed to 1.2-stable to address #11872.

comment:24 by Jun Omae, 3 years ago

I think the upgrade_tables method should call update_sequence if the table has an auto increment primary key.

in reply to:  24 ; comment:25 by Ryan J Ollos, 3 years ago

Replying to Jun Omae:

I think the upgrade_tables method should call update_sequence if the table has an auto increment primary key.

Is this only needed if the column with auto_increment has changed, or has been removed? If the column with auto_increment has not changed, then that column will have the same data in the new table as in the old.

in reply to:  25 comment:26 by Jun Omae, 3 years ago

Replying to Ryan J Ollos:

Replying to Jun Omae:

I think the upgrade_tables method should call update_sequence if the table has an auto increment primary key.

Is this only needed if the column with auto_increment has changed, or has been removed? If the column with auto_increment has not changed, then that column will have the same data in the new table as in the old.

The upgrade_tables re-create tables. Auto-increment column uses sequence object in PostgreSQL. Then, the sequence object would be re-created and started with 1. Inserting old records wouldn't change the sequence.

See also #8575.

Last edited 3 years ago by Jun Omae (previous) (diff)

comment:27 by Ryan J Ollos, 3 years ago

[7ea7e2d5/rjollos.git] calls update_sequence. I'll propose additional changes after #11872 is implemented.

comment:28 by Ryan J Ollos, 3 years ago

API Changes: modified (diff)

Fix for Cannot operate on closed database error committed to 1.0-stable in r15018, merged to 1.2-stable in r15019, merged to trunk in r15020.

DatabaseManager.upgrade_tables committed to 1.2-stable in r15021,r15023, merged to trunk in r15022,r15024.

Last edited 3 years ago by Ryan J Ollos (previous) (diff)

comment:29 by Ryan J Ollos, 3 years ago

API Changes: modified (diff)

comment:30 by Ryan J Ollos, 3 years ago

API Changes: modified (diff)
Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Committed to trunk in r15027, r15028, r15037. Eventually I'll start remembering to add files when pushing from a Git repos to SVN.

Last edited 3 years ago by Ryan J Ollos (previous) (diff)

comment:31 by Ryan J Ollos, 3 years ago

I was dumb and didn't run tests before committing r15037. To fix the broken build, I guess I need to create a temp directory:

  • trac/upgrades/tests/db42.py

     
    1212# history and logs, available at http://trac.edgewall.org/.
    1313
    1414import copy
     15import tempfile
    1516import unittest
    1617
    1718from trac.db.api import DatabaseManager
     
    5657class UpgradeTestCase(unittest.TestCase):
    5758
    5859    def setUp(self):
    59         self.env = EnvironmentStub()
     60        self.env = EnvironmentStub(path=tempfile.mkdtemp())
    6061        self.dbm = DatabaseManager(self.env)
    6162        with self.env.db_transaction:
    6263            self.dbm.drop_tables(new_schema)

Or instead just call self.env.reset_db() followed by self.env.shutdown()?

Last edited 3 years ago by Ryan J Ollos (previous) (diff)

comment:32 by Ryan J Ollos, 3 years ago

The test failure I introduced in r15037 was fixed in r15038. However, tests are still failing with on-disk SQLite. I suspect the pathway through destroy_db in db42.py has exposed an existing bug.

comment:33 by Jun Omae, 3 years ago

I think we should close connections before calling destroy_db

  • trac/test.py

    diff --git a/trac/test.py b/trac/test.py
    index ba8c7bb..93e809c 100755
    a b class EnvironmentStub(Environment):  
    384384                tables = dbm.reset_tables()
    385385            else:
    386386                # different version or version unknown, drop the tables
     387                if self.dburi.startswith('sqlite:') and \
     388                        self.dburi != 'sqlite::memory:':
     389                    # closes connections to avoid self.destroy_db() raises
     390                    # "WindowsError: [Error 32]" when deleting on-disk SQLite
     391                    dbm.shutdown()
    387392                self.destroy_db()
    388393
    389394        if not tables:

comment:34 by Ryan J Ollos, 3 years ago

Yeah, that looks good, I was working on a similar patch. I was hoping to keep the db specific information out of EnvironmentStub, but I don't see a way.

comment:35 by Ryan J Ollos, 3 years ago

It might be worth considering to put the logic in DatabaseManager.destroy_db(). I suppose a test would be useful to prove the case, but from looking at the code, wouldn't the logic be needed if there was a reason to destroy an on-disk SQLite database outside of EnvironmentStub?

comment:36 by Jun Omae, 3 years ago

Okay. Putting it in DatabaseManager.destroy_db() sounds good.

comment:37 by Ryan J Ollos, 3 years ago

Thanks, I'll go ahead and prepare that change, with tests.

comment:38 by Ryan J Ollos, 3 years ago

I added db_exists to DatabaseManager and made the changes to destroy_db: log:rjollos.git:t9612_destroydb. I haven't tested with PostgreSQL 8.x. Tests are passing on Travis CI and AppVeyor.

in reply to:  32 comment:39 by Jun Omae, 3 years ago

Replying to Ryan J Ollos:

The test failure I introduced in r15037 was fixed in r15038.

Running unit tests with r15037 while investigating the failure, the EnvironmentStub.reset_db_and_disk() accidentally removed trac in my source directory. EnvironmentStub.path is set to os.path.dirname(trac.__file__) if path argument is empty at tags/trac-1.0.12/trac/test.py@:360-365#L332.

>>> env = EnvironmentStub()
>>> env.path
'/src/tracdev/git/trac'

I think we should check whether self.path is in current directory to avoid the accident before removing self.path directory.

  • trac/test.py

    diff --git a/trac/test.py b/trac/test.py
    index 9a7a30e7e..9ee756cc5 100755
    a b class EnvironmentStub(Environment):  
    475475        """
    476476        self.reset_db()
    477477        self.shutdown() # really closes the db connections
    478         shutil.rmtree(self.path)
     478        cwd = os.getcwd()
     479        # avoid removing source directory
     480        if os.path.commonprefix([cwd, self.path]) != cwd:
     481            shutil.rmtree(self.path)
    479482        if self._old_registry is not None:
    480483            self.restore_component_registry()
    481484

comment:40 by Ryan J Ollos, 3 years ago

I've encountered that problem as well. It might not be an issue, but an environment created in the root of the source directory would not be removed.

The reason I started thinking about that was remembering that the functional tests, by default, create the environment in <path to source>/testenv/trac: trunk/trac/tests/functional/__init__.py@14794:133-138#L122. The functional tests don't call reset_db_and_disk, so your patch doesn't create a problem in this case.

It's probably bad practice to create an environment directory in the source tree, so I think the patch is okay. I have two thoughts on the functional test cases though:

  • Should we destroy the environment at the end of the test run?
  • Should we put the environment in a temp directory? If putting in a temp directory, we might want to randomize the directory name.

Also, adding a mkdir function in trac.test, we could ensure use of the same prefix for all temporary directories.

log:rjollos.git:t9612_destroy_functional_testenv

in reply to:  38 ; comment:41 by Jun Omae, 3 years ago

Replying to Ryan J Ollos:

I added db_exists to DatabaseManager and made the changes to destroy_db: log:rjollos.git:t9612_destroydb. I haven't tested with PostgreSQL 8.x. Tests are passing on Travis CI and AppVeyor.

Reconsidering, we could simply call self.shutdown() before connector.destroy_db() because destroy_db directly connects a database without ConnectionPool: [bbe878e89/jomae.git]

in reply to:  41 ; comment:42 by Ryan J Ollos, 3 years ago

Replying to Jun Omae:

Reconsidering, we could simply call self.shutdown() before connector.destroy_db() because destroy_db directly connects a database without ConnectionPool: [bbe878e89/jomae.git]

That is much cleaner. I didn't even consider that it might work, but it's definitely the better solution.

in reply to:  40 comment:43 by Jun Omae, 3 years ago

Replying to Ryan J Ollos:

I have two thoughts on the functional test cases though:

  • Should we destroy the environment at the end of the test run?
  • Should we put the environment in a temp directory? If putting in a temp directory, we might want to randomize the directory name.

When an exception is raised from twill, the html is saved as <test-case-name>.html in log directory of the test environment. I think it would be better to keep the environment if tests are failing, at least.

comment:44 by Ryan J Ollos, 3 years ago

Good point. I added additional changes to consider:

  • Create temporary dir for functional test run in system temp directory, but don't delete it at the end of a test run: [8788908e/rjollos.git].
  • Use the system temp directory as the default for EnvironmentStub.path, which would make the comment:39 change unnecessary: [808e806f/rjollos.git].

comment:45 by Jun Omae, 3 years ago

I consider the following code in [808e806f/rjollos.git] would remove all owned files in the temporary directory if the path of EnvironmentStub.__init__ is None and reset_db_and_disk() is wrongly called in tearDown().

            self.path = tempfile.gettempdir()
$ mkdir /dev/shm/t9612
$ touch /dev/shm/t9612/test.txt
$ ls -al /dev/shm/t9612/
total 0
drwxr-xr-x  2 jun66j5 jun66j5  60 Aug  5 22:22 .
drwxrwxrwt 20 root    root    440 Aug  5 22:20 ..
-rw-r--r--  1 jun66j5 jun66j5   0 Aug  5 22:22 test.txt
$ TMP=/dev/shm/t9612 python
Python 2.7.6 (default, Jun 22 2015, 17:58:13)
[GCC 4.8.2] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> import tempfile
>>> from trac.test import EnvironmentStub
>>> tempfile.gettempdir()
'/dev/shm/t9612'
>>> env = EnvironmentStub()
>>> env.path
'/dev/shm/t9612'
>>> env.reset_db_and_disk()
>>> os.listdir(env.path)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: [Errno 2] No such file or directory: '/dev/shm/t9612'

comment:46 by Ryan J Ollos, 3 years ago

I added changes to address comment:45 in [8b14dd6b/rjollos.git].

Last edited 3 years ago by Ryan J Ollos (previous) (diff)

comment:47 by Jun Omae, 3 years ago

If unit tests run with on-disk SQLite file, we generally use TRAC_TEST_DB_URI=sqlite:test.db. With the branch, /tmp/test.db would be created (currently, trac/test.db is created in source tree). Then, only unit tests could run in the same time on system.

in reply to:  42 comment:48 by Ryan J Ollos, 3 years ago

Replying to Ryan J Ollos:

Replying to Jun Omae:

Reconsidering, we could simply call self.shutdown() before connector.destroy_db() because destroy_db directly connects a database without ConnectionPool: [bbe878e89/jomae.git]

That is much cleaner. I didn't even consider that it might work, but it's definitely the better solution.

Committed to 1.2-stable in r15042, merged to trunk in r15043. I tested with PostgreSQL 8.4.20 on CentOS 6.8.

in reply to:  40 comment:49 by Ryan J Ollos, 3 years ago

Replying to Ryan J Ollos:

Also, adding a mkdir function in trac.test, we could ensure use of the same prefix for all temporary directories.

Committed to trunk in r15050 (commit message should say mkdtemp rather than mkdir).

comment:50 by Jun Omae, 3 years ago

I think DatabaseManager.upgrade_tables should quote table name and column names using db.quote().

  • trac/db/api.py

    diff --git a/trac/db/api.py b/trac/db/api.py
    index e08b912bc..6c6e98ac0 100644
    a b class DatabaseManager(Component):  
    425425        """
    426426        for new_table in new_schema:
    427427            temp_table_name = new_table.name + '_old'
    428             old_column_names = set(self.get_column_names(new_table))
    429             new_column_names = set(col.name for col in new_table.columns)
    430             cols_to_copy = ','.join(old_column_names & new_column_names)
     428            column_names = (set(self.get_column_names(new_table)) &
     429                            set(col.name for col in new_table.columns))
    431430            with self.env.db_transaction as db:
     431                cols_to_copy = ','.join(db.quote(name)
     432                                        for name in column_names)
    432433                cursor = db.cursor()
    433434                cursor.execute("""
    434435                    CREATE TEMPORARY TABLE %s AS SELECT * FROM %s
    435                     """ % (temp_table_name, new_table.name))
     436                    """ % (db.quote(temp_table_name),
     437                           db.quote(new_table.name)))
    436438                self.drop_tables((new_table,))
    437439                self.create_tables((new_table,))
    438440                cursor.execute("""
    439441                    INSERT INTO %s (%s) SELECT %s FROM %s
    440                     """ % (new_table.name, cols_to_copy,
    441                            cols_to_copy, temp_table_name))
     442                    """ % (db.quote(new_table.name), cols_to_copy,
     443                           cols_to_copy, db.quote(temp_table_name)))
    442444                for col in new_table.columns:
    443445                    if col.auto_increment:
    444446                        db.update_sequence(cursor, new_table.name, col.name)

comment:51 by Jun Omae, 3 years ago

Minor issue: if there is no common columns between old and new tables, INSERT INTO %s (%s) SELECT %s FROM %s would fail to execute.

Last edited 3 years ago by Jun Omae (previous) (diff)

comment:52 by Ryan J Ollos, 3 years ago

API Changes: modified (diff)

comment:53 by Ryan J Ollos, 3 years ago

Proposed changes look good to me.

Last edited 3 years ago by Ryan J Ollos (previous) (diff)

in reply to:  53 comment:54 by Jun Omae, 3 years ago

Replying to Ryan J Ollos:

Proposed changes look good to me.

Thanks for the reviewing. Committed in [15317] and merged in [15318].

by Jun Omae, 3 years ago

in reply to:  51 comment:55 by Jun Omae, 3 years ago

Replying to Jun Omae:

Minor issue: if there is no common columns between old and new tables, INSERT INTO %s (%s) SELECT %s FROM %s would fail to execute.

Proposed changes in t9612-no-common-columns.diff.

comment:56 by Jun Omae, 3 years ago

I noticed .upgrade_table creates transaction per each table. It would be better to create one transaction for call of .upgrade_table?

  • trac/db/api.py

     
    423423
    424424        :since: version 1.2
    425425        """
    426         for new_table in new_schema:
    427             temp_table_name = new_table.name + '_old'
    428             old_column_names = set(self.get_column_names(new_table))
    429             new_column_names = set(col.name for col in new_table.columns)
    430             column_names = old_column_names & new_column_names
    431             with self.env.db_transaction as db:
     426        with self.env.db_transaction as db:
     427            cursor = db.cursor()
     428            for new_table in new_schema:
     429                temp_table_name = new_table.name + '_old'
     430                old_column_names = set(self.get_column_names(new_table))
     431                new_column_names = set(col.name for col in new_table.columns)
     432                column_names = old_column_names & new_column_names
    432433                cols_to_copy = ','.join(db.quote(name)
    433434                                        for name in column_names)
    434                 cursor = db.cursor()
    435435                cursor.execute("""
    436436                    CREATE TEMPORARY TABLE %s AS SELECT * FROM %s
    437437                    """ % (db.quote(temp_table_name),
Last edited 3 years ago by Jun Omae (previous) (diff)

comment:57 by Ryan J Ollos, 3 years ago

Both changes looks good.

comment:58 by Jun Omae, 3 years ago

Thanks. Committed in [15319] and merged in [15320].

comment:59 by Ryan J Ollos, 14 months ago

Fixed one oversight in r16784. The test is currently unused.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.