v.out.ogr: Append mode broken in G 7.2.1svn and 7.3

[sbl]

The following works in G 7.0 but not in G 7.2.1svn and G 7.3

v.out.ogr --q -m output=test.sqlite olayer=test input=soils_wake lco="OVERWRITE=YES,GEOMETRY_NAME=geom,FID=fid,LAUNDER=YES" format=SQLite
v.out.ogr --q -mua output=test.sqlite olayer=test input=soils_wake lco="OVERWRITE=YES,GEOMETRY_NAME=geom,FID=fid,LAUNDER=YES" format=SQLite

Error message on second command in G 7.2.1/7.3 is:

ERROR: option <output>: <test.sqlite> exists. To overwrite, use the --overwrite flag

However, with --o the layer to append is deleted first and the following eror message appears:

WARNING: OGR layer <test> already exists and will be overwritten
ERROR: OGR layer <test> not found

Probably related to: ​https://trac.osgeo.org/grass/changeset/70204/ ?

[sbl]

As a test I reverted locally ​https://trac.osgeo.org/grass/changeset/70204/#file0 After that append mode works again in G7.2.1. So, it seems changeset 70204 broke it...

Maybe the change should be reverted until a less conflicting solution is found? I would refer the append functionality over a GUI button...

[martinl]

In 70500:

v.out.ogr: Append mode broken in G 7.2.1svn and 7.3 (see #3270)

[martinl]

Should be solved in r70500. It changes API a bit, the Flag structure has new attribute - suppress_overwrite. For relb72 we have two options:

a) backport (API change) b) revert r70204.

I would vote for a) since GUI dialog is quite unusable since you need to write path to output file by hand. Any opinion?

[mlennert]

Replying to martinl:

Should be solved in r70500. It changes API a bit, the Flag structure has new attribute - suppress_overwrite. For relb72 we have two options:

a) backport (API change) b) revert r70204.

I would vote for a) since GUI dialog is quite unusable since you need to write path to output file by hand. Any opinion?

I wouldn't call it unusable (many of use have used it in that form for years), just not very user-friendly. ;-)

IIUC, this is not a change of the user-visible API, but of the internal API, or ? Then I don't have an issue with that.

However, (without having looked into this in detail) why can't this "just" be directly dealt with within v.out.ogr main.c ?

Wouldn't something like this do the job ? :

Index: main.c
===================================================================
--- main.c	(révision 70501)
+++ main.c	(copie de travail)
@@ -488,23 +488,25 @@
     /* check if OGR layer exists */
     overwrite = G_check_overwrite(argc, argv);
     found = FALSE;
-    for (i = 0; i < OGR_DS_GetLayerCount(Ogr_ds); i++) {
-	Ogr_layer = OGR_DS_GetLayer(Ogr_ds, i);
-	Ogr_field = OGR_L_GetLayerDefn(Ogr_layer);
-	if (G_strcasecmp(OGR_FD_GetName(Ogr_field), options.layer->answer))
-	    continue;
-	
-	found = TRUE;
-	if (!overwrite && !flags.append->answer) {
-	    G_fatal_error(_("Layer <%s> already exists in OGR data source '%s'"),
-			  options.layer->answer, options.dsn->answer);
+    if (!flags.append->answer) {
+	for (i = 0; i < OGR_DS_GetLayerCount(Ogr_ds); i++) {
+	    Ogr_layer = OGR_DS_GetLayer(Ogr_ds, i);
+	    Ogr_field = OGR_L_GetLayerDefn(Ogr_layer);
+	    if (G_strcasecmp(OGR_FD_GetName(Ogr_field), options.layer->answer))
+		continue;
+	    
+	    found = TRUE;
+	    if (!overwrite) {
+		G_fatal_error(_("Layer <%s> already exists in OGR data source '%s'"),
+			      options.layer->answer, options.dsn->answer);
+	    }
+	    else if (overwrite) {
+		G_warning(_("OGR layer <%s> already exists and will be overwritten"),
+			  options.layer->answer);
+		OGR_DS_DeleteLayer(Ogr_ds, i);
+		break;
+	    }
 	}
-	else if (overwrite) {
-	    G_warning(_("OGR layer <%s> already exists and will be overwritten"),
-		      options.layer->answer);
-	    OGR_DS_DeleteLayer(Ogr_ds, i);
-	    break;
-	}
     }
     if (flags.append->answer && !found) {
 	G_warning(_("OGR layer <%s> doesn't exists, "

since we don't need to worry about overwriting at all when we are in append mode, or ?

[martinl]

Replying to mlennert:

IIUC, this is not a change of the user-visible API, but of the internal API, or ? Then I don't have an issue with that.

https://grass.osgeo.org/programming7/structFlag.html

Wouldn't something like this do the job ? :

Probably yes. My intend was to introduce generic solution how to suppress overwrite check. Any idea about other modules which could have benefit from that? If there is only G72:v.out.ogr we could probably go for local modification as you suggests.

[mlennert]

Replying to martinl:

Replying to mlennert:

IIUC, this is not a change of the user-visible API, but of the internal API, or ? Then I don't have an issue with that.

https://grass.osgeo.org/programming7/structFlag.html

Yes, this is what I call the internal API. Users are generally not confronted with this. In my understanding our no-API change rule is about module API, not internal API, as GRASS GIS is generally not used as a library.

Wouldn't something like this do the job ? :

Probably yes. My intend was to introduce generic solution how to suppress overwrite check. Any idea about other modules which could have benefit from that? If there is only G72:v.out.ogr we could probably go for local modification as you suggests.

I can't think of another module where one specific choice warrants an override of overwrite settings...

[martinl]

Replying to mlennert:

Wouldn't something like this do the job ? :

this patch worked for you? The overwrite check is done earlier by parser (since output is not G_OPT_F_OUTPUT)?

[mlennert]

Replying to martinl:

Replying to mlennert:

Wouldn't something like this do the job ? :

this patch worked for you?

I didn't try. It was just the result of a quick look at the code, not a real patch.

The overwrite check is done earlier by parser (since output is not G_OPT_F_OUTPUT)?

Yes, right. So, you are probably right that this needs a parser-level solution.

Sorry for the noise...

[martinl]

In 70617:

libgis: add suppress overwrite attribute (see #3270)

[martinl]

In 70618:

v.out.ogr: Append mode broken in G 7.2.1svn and 7.3 (closes #3270)