mantisbt-plugins / source-integration

Source control integration plugin framework for MantisBT, including support for Github, Gitlab, Bitbucket, Gitea, Gitweb, Cgit, Subversion, Mercurial and more
http://noswap.com/projects/source-integration/
MIT License
181 stars 130 forks source link

Error when opening repositories management page with SQL Server #36

Open jeckyhl opened 11 years ago

jeckyhl commented 11 years ago

My configuration is MantisBT 1.2.12 with SQL Server 2008

An error occurs when opening repositories management page : operand text is not valid for count operator

the query is

SELECT COUNT(DISTINCT filename) FROM mantis_plugin_Source_file_table AS f
JOIN mantis_plugin_Source_changeset_table AS c
ON c.id=f.change_id
WHERE c.repo_id=?

My workaround is to use VARCHAR column type instead of TEXT

ALTER TABLE mantis_plugin_Source_file_table
ALTER COLUMN filename VARCHAR(250) NOT NULL

Maybe VARCHAR(MAX) would be better ?

dregad commented 11 years ago

I would tend to agree with your proposed fix, but I would advise to use VARCHAR(255) and not 250, as this is the maximum length for a filename in most file systems [1].

However, I am not really sure why @jreese set the filename column's type to TEXT instead of VARCHAR.

John, care to comment ? Do you see any issue with changing the schema definition ?

[1] http://en.wikipedia.org/wiki/Comparison_of_file_systems#Limits

amyreese commented 11 years ago

It's a TEXT field because it stores more than just the individual filename, it also stores the entire file path from the root of the repository, which can easily and quickly exceed 255 characters. I would be hesitant to suggest changing it from TEXT for those reasons, especially since the majority of users will be running mysql or postgres where that query functions correctly. I'm not familiar with SQL Server, so I have no idea of what is or isn't possible there, but for that specific query instance, it's not really "necessary" as it just provides a bit of stats for the repo list, so could simply be omitted in cases where the DB doesn't support those semantics.

jeckyhl commented 11 years ago

VARCHAR(MAX) would do the trick -- you can store as many characters as you want (up to 2 Go) in VARCHAR(MAX) field

As it is stated here:

The VARCHAR(MAX) type is a replacement for TEXT. The basic difference is that a TEXT type will always store the data in a blob whereas the VARCHAR(MAX) type will attempt to store the data directly in the row unless it exceeds the 8k limitation and at that point it stores it in a blob.

So

amyreese commented 11 years ago

Then I would suggest modifying the plugin's schema() section to check the database type, and modify the schema for that column to use varchars only on sql server. I would advise against adding a new ALTER schema row though, in favor of just returning a modified CREATE row, so that schema versions aren't out of sync between various database types. Ideally, only the initial CREATE row would be part of the conditional block, which might require altering the semantics of the block, but would be better IMO than duplicating the entire schema array entirely.

dregad commented 11 years ago

Thanks for the feedback John.

It's a TEXT field because it stores more than just the individual filename, it also stores the entire file path from the root of the repository, which can easily and quickly exceed 255 characters

In that case, the limit would be the maximum path length, which is 4096 in Linux and 32767 on NTFS. But indeed as you say, the best approach is probably to keep it as TEXT and make an MSSQL-specific schema - assuming ADOdb can actually handle this VARCHAR(MAX) thing.

@jeckyhl

I would advise against adding a new ALTER schema row though, in favor of just returning a modified CREATE row, so that schema versions aren't out of sync between various database types.

That would potentially cause issues for existing installations, unless they manually execute an ALTER TABLE, no ?

What do you think about adding a schema change for MSSQL, and a no-op upgrade step for other DB's (similar to what I did in my MantisBT oracle branch ?

amyreese commented 11 years ago

My question is whether that sort of null/noop schema row is supported by the plugin manager, which uses a slightly different schema definition/system than the core of Mantis.

dregad commented 11 years ago

My question is whether that sort of null/noop schema row is supported by the plugin manager

You're right, it wouldn't be supported in the current state of the code.

For Oracle branch I had to modify install.php to take care of this no-op, so to make this work for plugins the same change would have to be made in plugin_api.php / plugin_upgrade()

jeckyhl commented 11 years ago

I can't help about the schema upgrade problematic.. it seems to be more tricky than one could expect !

@dregad about your two questions:

is VARCHAR(MAX) valid for all MSSQL versions, or is it specific to 2008 only

VARCHAR(MAX) has been introduced in SQL Server 2005. It is also available in SQL Server 2012. Since it is meant to be a replacement for TEXT, I will be certainly supported in futures versions of SQL Server.

would you be able and willing to test ? I don't have access to an MSSQL platform

I switched to VARCHAR(MAX) on our Mantis production site (since John said VARCHAR(255) was too short), it works fine (but I'm not sure about the mssql driver we use). I can do some more tests if needed