Opened 5 years ago

Closed 4 years ago

#4318 closed defect (fixed)

MSSQLSpatial incorrectly assumes an "image" type column to be geometry

Reported by: danhomerick Owned by: tamas
Priority: normal Milestone:
Component: OGR_SF Version: svn-trunk
Severity: normal Keywords: MSSQLSpatial
Cc: tamas

Description

When the geometry column is not explicitly named, the MS SQL Spatial format attempts to autodetect which column contains geometry data. The autodetection logic assumes that any "image" type column (a subtype of binary data) contains WKB. Because the "image" type, unlike the "geometry" and "geography" column types is not exclusive to spatial data, this assumption is risky.

At connection, the spatial storage format is specified (WKB, WKT, or Native), defaulting to Native. A patch is attached which propagates which storage format is being used down to the autodetection logic, and changes the logic so as to only make the risky assumption about an "image" column when the spatial data is stored in WKB. It also allows for WKB which is stored in a "varbinary" column type, since the "image" column type is deprecated (since SQL Server 2005 [1]).

A more comprehensive fix might be to attempt decoding some of the "image" column, in order to check that it actually contains WKB.

The patch does not address autodetecting a column containing WKT, which was also not addressed by the original code.

An alternate fix might be to remove autodetection of geometry columns entirely unless stored in the unambiguous "geometry" or "geography" types (i.e. native).

[1] http://msdn.microsoft.com/en-us/library/ms187993.aspx

Attachments (1)

issue4318.patch (14.2 KB) - added by danhomerick 5 years ago.

Download all attachments as: .zip

Change History (11)

Changed 5 years ago by danhomerick

Attachment: issue4318.patch added

comment:1 Changed 5 years ago by Even Rouault

Cc: tamas added
Component: defaultOGR_SF
Keywords: MSSQLSpatial added

comment:2 Changed 5 years ago by danhomerick

The supplied patch is fundamentally flawed, and should be discarded. It was based on the incorrect assumption that the type name returned by SQLColAttribute (specifying the SQL_DESC_TYPE_NAME attribute) is the same as the type name used when the table was created (e.g. "image", "geometry" or "geography". It is not.

I was initially misled by the test table that I was working with, which had an "image" column, for which the type name did happen to match. I didn't test the patch sufficiently to realize my mistake until now. Mea culpa.

I'm not marking the issue as invalid, because the problem with a binary, non-spatial column being wrongly assumed to contain spatial data does still exist. But I wouldn't object if the issue was closed, and quietly buried. It's pretty embarrassing, really.

comment:3 Changed 5 years ago by tamas

Owner: changed from warmerdam to tamas

comment:4 Changed 5 years ago by tamas

Could you provide the table definition which can lead to this condition?

comment:5 Changed 5 years ago by danhomerick

Sure. Slightly shortened from what I was using, but:

CREATE TABLE [dbo].[Table1] (

[ID] [int] IDENTITY (1, 1) NOT NULL , [foo] [int] NULL , [bar] [image] NULL

) ON [PRIMARY] GO

Note that "bar" does not containing spatial data.

comment:6 Changed 5 years ago by tamas

And how did you do the query against this non spatial table? I'm not sure the queries will require to work on such tables that don't have any geometry columns.

comment:7 Changed 5 years ago by danhomerick

I don't have the exact code in front of me, but something like:

OGRLayer *layer = dataSource.executeSQL("SELECT * FROM Table1", NULL, NULL); OGRFeature *feature = layer->GetNextFeature?(); int fooValue = feature->GetFieldAsInteger?(0); int sizeOfBar; GByte *barValue = feature->GetFieldAsBinary?(1, &sizeOfBar);

The documentation doesn't suggest that this won't work. More to the point, from a usability standpoint it would be really nice if we could get this to work. Since it's not using the "OGRSQL" dialect, it seems reasonable to hope that the SQL query could be "passed directly through to MSSQL" and get usable results.

comment:8 Changed 5 years ago by danhomerick

Corrected for wiki formatting, and to include the id field:

OGRLayer *layer = dataSource.executeSQL("SELECT * FROM Table1", NULL, NULL);
OGRFeature *feature = layer->GetNextFeature();
int idValue = feature->GetFieldAsInteger(0);
int fooValue = feature->GetFieldAsInteger(1);
int sizeOfBar;
GByte *barValue = feature->GetFieldAsBinary(2, &sizeOfBar);

comment:9 Changed 4 years ago by tamas

Fixed in trunk (r25989)

comment:10 Changed 4 years ago by tamas

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.