DynamicForm executes ADD instead of UPDATE, resulting in UC violation

[Blama]

Hi Isomorphic,

please see this builtInDS based testcase (v10.0p_2015-04-30, FF26 dev mode):

BuiltInDS.java:

package com.smartgwt.sample.client;

import com.google.gwt.core.client.EntryPoint;
import com.smartgwt.client.core.KeyIdentifier;
import com.smartgwt.client.data.AdvancedCriteria;
import com.smartgwt.client.data.Criterion;
import com.smartgwt.client.data.DSCallback;
import com.smartgwt.client.data.DSRequest;
import com.smartgwt.client.data.DSResponse;
import com.smartgwt.client.data.DataSource;
import com.smartgwt.client.types.OperatorId;
import com.smartgwt.client.util.PageKeyHandler;
import com.smartgwt.client.util.Page;
import com.smartgwt.client.util.SC;
import com.smartgwt.client.widgets.IButton;
import com.smartgwt.client.widgets.events.ClickEvent;
import com.smartgwt.client.widgets.events.ClickHandler;
import com.smartgwt.client.widgets.form.DynamicForm;
import com.smartgwt.client.widgets.form.fields.FormItem;
import com.smartgwt.client.widgets.form.fields.HiddenItem;
import com.smartgwt.client.widgets.layout.HLayout;
import com.smartgwt.client.widgets.layout.VStack;

public class BuiltInDS implements EntryPoint {
	private DynamicForm boundForm;
	private FormItem itemID;
	private IButton saveBtn;
	private IButton cancelBtn;

	public void onModuleLoad() {
		KeyIdentifier debugKey = new KeyIdentifier();
		debugKey.setCtrlKey(true);
		debugKey.setKeyName("D");

		Page.registerKey(debugKey, new PageKeyHandler() {
			public void execute(String keyName) {
				SC.showConsole();
			}
		});

		VStack vStack = new VStack();
		vStack.setLeft(175);
		vStack.setTop(75);
		vStack.setWidth("70%");
		vStack.setMembersMargin(20);

		boundForm = new DynamicForm();
		boundForm.setDataSource(DataSource.get("supplyItem"));

		itemID = new FormItem("itemID");

		FormItem itemName = new FormItem("itemName");
		itemName.setDefaultValue("Default name that might be OK");

		FormItem sku = new FormItem("SKU");
		sku.setDefaultValue("DefaultOK");

		FormItem category = new FormItem("category");
		category.setDefaultValue("Default category that might be OK");

		FormItem unitCost = new FormItem("unitCost");
		unitCost.setDefaultValue(42);

		boundForm.setFields(itemID, itemName, sku, category, unitCost);
		boundForm.fetchData(new AdvancedCriteria(new Criterion("itemID", OperatorId.EQUALS, 10004)), new DSCallback() {
			@Override
			public void execute(DSResponse dsResponse, Object data, DSRequest dsRequest) {
				if (dsResponse.getStatus() == DSResponse.STATUS_SUCCESS && dsResponse.getTotalRows() == 1)
					boundForm.rememberValues();
				else if (dsResponse.getStatus() == DSResponse.STATUS_SUCCESS && dsResponse.getTotalRows() == 0) {
					boundForm.rememberValues();
					itemID.setValue(10004);
					SC.say("PK value set via code");
				}
			}
		});

		vStack.addMember(boundForm);

		HLayout hLayout = new HLayout(10);
		hLayout.setMembersMargin(10);
		hLayout.setHeight(22);

		saveBtn = new IButton("Save");
		saveBtn.addClickHandler(new ClickHandler() {
			public void onClick(ClickEvent event) {
				boundForm.saveData();
			}
		});
		hLayout.addMember(saveBtn);

		cancelBtn = new IButton("Status?");
		cancelBtn.addClickHandler(new ClickHandler() {
			public void onClick(ClickEvent event) {
				if (boundForm.valuesHaveChanged()) {
					SC.say("Values have changed");
					// boundForm.resetValues();
				} else
					SC.say("Values have not changed");
			}
		});
		hLayout.addMember(cancelBtn);
		vStack.addMember(hLayout);
		vStack.draw();
	}
}

supplyItem.ds.xml (removed sequence, removed invalid FK):

<DataSource ID="supplyItem" serverType="sql" tableName="supplyItem" titleField="itemName" testFileName="/examples/shared/ds/test_data/supplyItem.data.xml"
	dbImportFileName="/examples/shared/ds/test_data/supplyItemLarge.data.xml">
	<fields>
		<field name="itemID" type="integer" hidden="true" primaryKey="true" />
		<field name="itemName" type="text" title="Item" length="128" required="true" />
		<field name="SKU" type="text" title="SKU" length="10" required="true" />
		<field name="description" type="text" title="Description" length="2000" />
		<field name="category" type="text" title="Category" length="128" required="true" />
		<field name="units" type="enum" title="Units" length="5">
			<valueMap>
				<value>Roll</value>
				<value>Ea</value>
				<value>Pkt</value>
				<value>Set</value>
				<value>Tube</value>
				<value>Pad</value>
				<value>Ream</value>
				<value>Tin</value>
				<value>Bag</value>
				<value>Ctn</value>
				<value>Box</value>
			</valueMap>
		</field>
		<field name="unitCost" type="float" title="Unit Cost" required="true">
			<validators>
				<validator type="floatRange" min="0" errorMessage="Please enter a valid (positive) cost" />
				<validator type="floatPrecision" precision="2" errorMessage="The maximum allowed precision is 2" />
			</validators>
		</field>
		<field name="inStock" type="boolean" title="In Stock" />
		<field name="nextShipment" type="date" title="Next Shipment" />
	</fields>
</DataSource>

Start the sample and hit "Save" twice to see the effect (also included as screenshot). Observations:
- This does not happen if the PK-FormItem itemID is a HiddenItem instead of a FormItem.
- On reload after save (when the fetch returns data), the correct action of UPDATE is triggered on save-button hit.

Best regards
Blama

[Blama]

As note: The bug does not affect me directly, I just noticed it while trying to create a testcase. So it has no priority for me.

[Isomorphic]

The docs explain how the form chooses add vs update, and how you can explicitly specify it in cases where the form doesn't have enough information to decide reliably.

If you think there's a bug here, please explain how the behavior is not matching the docs.

[Blama]

Hi Isomorphic,

I found DynamicForm.getSaveOperationType().

If saveOperationType is unset, the form will heuristically determine whether an "add" or "update" operation is intended based on whether the primaryKey field is present and editable.

With respect to this, the testcase is working as described in the docs in both cases (FormItem and HiddenItem).

Thank you & Best regards
Blama

[Blama]

Hi Isomorphic,

sorry to reopen this:
In the normal BuiltInDS (v10.0p_2015-05-19) this is not working. Select Animals, Click "Scientific Name" to sort by that column and select "Allligator mississippiensis". Change PK-value to "Allligator mississippiensis2" and hit "Save".

The dynamic form issues an UPDATE, but for "Allligator mississippiensis2".
In result the server-side update fails (0 rows updated) and in the the row gets doubled, once with the "2"-suffix, once without.

Client log:
Request:

{
    dataSource:"animals", 
    operationType:"update", 
    componentId:"isc_DynamicForm_0", 
    data:{
        picture:"Alligator.jpg", 
        information:"In the 16th century, Spanish explorers in what is now Florida encountered a large formidable animal which they called \"el largo\" meaning \"the lizard\". The name \"el largo\" gradually became pronounced \"alligator\".", 
        commonName:"Alligator (American)", 
        lifeSpan:50, 
        scientificName:"Allligator mississippiensis2", 
        diet:"Carnivore", 
        status:"Not Endangered"
    }, 
    textMatchStyle:"exact", 
    callback:{
        target:[DynamicForm ID:isc_DynamicForm_0], 
        methodName:"saveEditorReply"
    }, 
    showPrompt:true, 
    prompt:"Saving form...", 
    oldValues:{
        picture:"Alligator.jpg", 
        information:"In the 16th century, Spanish explorers in what is now Florida encountered a large formidable animal which they called \"el largo\" meaning \"the lizard\". The name \"el largo\" gradually became pronounced \"alligator\".", 
        commonName:"Alligator (American)", 
        lifeSpan:50, 
        scientificName:"Allligator mississippiensis", 
        diet:"Carnivore", 
        status:"Not Endangered"
    }, 
    requestId:"animals$62724", 
    internalClientContext:{
    }, 
    fallbackToEval:false, 
    afterFlowCallback:"isc_DynamicForm_0.$49z(dsRequest, dsResponse, data)", 
    editor:[DynamicForm ID:isc_DynamicForm_0], 
    lastClientEventThreadCode:"MUP3", 
    bypassCache:true
}

Result:

{
    affectedRows:0, 
    invalidateCache:false, 
    isDSResponse:true, 
    operationType:"update", 
    queueStatus:0, 
    status:0, 
    data:null
}

Server log:

=== 2015-05-20 15:29:33,485 [c-47] INFO  RequestContext - URL: '/BuiltInDS/builtinds/sc/IDACall', User-Agent: 'Mozilla/5.0 (Windows NT 6.3; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0': Moz (Gecko) with Accept-Encoding header
=== 2015-05-20 15:29:33,486 [c-47] DEBUG IDACall - Header Name:Value pair: host:localhost:8080
=== 2015-05-20 15:29:33,486 [c-47] DEBUG IDACall - Header Name:Value pair: user-agent:Mozilla/5.0 (Windows NT 6.3; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
=== 2015-05-20 15:29:33,486 [c-47] DEBUG IDACall - Header Name:Value pair: accept:text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
=== 2015-05-20 15:29:33,486 [c-47] DEBUG IDACall - Header Name:Value pair: accept-language:de-de,de;q=0.8,en-us;q=0.5,en;q=0.3
=== 2015-05-20 15:29:33,486 [c-47] DEBUG IDACall - Header Name:Value pair: accept-encoding:gzip, deflate
=== 2015-05-20 15:29:33,486 [c-47] DEBUG IDACall - Header Name:Value pair: dnt:1
=== 2015-05-20 15:29:33,486 [c-47] DEBUG IDACall - Header Name:Value pair: content-type:application/x-www-form-urlencoded; charset=UTF-8
=== 2015-05-20 15:29:33,486 [c-47] DEBUG IDACall - Header Name:Value pair: referer:http://localhost:8080/BuiltInDS/BuiltInDS.html
=== 2015-05-20 15:29:33,486 [c-47] DEBUG IDACall - Header Name:Value pair: content-length:2611
=== 2015-05-20 15:29:33,486 [c-47] DEBUG IDACall - Header Name:Value pair: cookie:JSESSIONID=C5631D101CCCE887D2DBB40F36F47C69; isc_cState=ready; GLog=%7B%0D%20%20%20%20trackRPC%3Atrue%2C%20%0D%20%20%20%20pageGUID%3A%2241645E28-A8A9-4B95-822F-5C44EE205904%22%2C%20%0D%20%20%20%20priorityDefaults%3A%7B%0D%20%20%20%20%20%20%20%20sgwtInternal%3A1%0D%20%20%20%20%7D%2C%20%0D%20%20%20%20defaultPriority%3A3%2C%20%0D%20%20%20%20left%3A862%2C%20%0D%20%20%20%20top%3A130%2C%20%0D%20%20%20%20width%3A940%2C%20%0D%20%20%20%20height%3A519%0D%7D
=== 2015-05-20 15:29:33,486 [c-47] DEBUG IDACall - Header Name:Value pair: connection:keep-alive
=== 2015-05-20 15:29:33,487 [c-47] DEBUG IDACall - Header Name:Value pair: pragma:no-cache
=== 2015-05-20 15:29:33,487 [c-47] DEBUG IDACall - Header Name:Value pair: cache-control:no-cache
=== 2015-05-20 15:29:33,487 [c-47] DEBUG IDACall - session exists: C5631D101CCCE887D2DBB40F36F47C69
=== 2015-05-20 15:29:33,487 [c-47] DEBUG IDACall - remote user: null
=== 2015-05-20 15:29:33,490 [c-47] DEBUG XML - Parsed XML from (in memory stream): 2ms
=== 2015-05-20 15:29:33,491 [c-47] DEBUG ISCKeyedObjectPool - Borrowing object for 'transaction'
=== 2015-05-20 15:29:33,491 [c-47] DEBUG PoolableDataSourceFactory - Created DataSource null of type 'transaction' in the pooling flow
=== 2015-05-20 15:29:33,491 [c-47] DEBUG ISCKeyedObjectPool - Borrowing object for 'Object'
=== 2015-05-20 15:29:33,492 [c-47] DEBUG PoolableDataSourceFactory - Created DataSource 169 of type 'Object' and assigned it to thread http-bio-8080-exec-47
=== 2015-05-20 15:29:33,492 [c-47] DEBUG PoolableDataSourceFactory - Created DataSource 169 of type 'Object' in the pooling flow
=== 2015-05-20 15:29:33,492 [c-47] DEBUG PoolableDataSourceFactory - Activated DataSource 169 of type 'Object'
=== 2015-05-20 15:29:33,492 [c-47] DEBUG ISCKeyedObjectPool - Borrowing object for 'List'
=== 2015-05-20 15:29:33,493 [c-47] DEBUG PoolableDataSourceFactory - Created DataSource 170 of type 'List' and assigned it to thread http-bio-8080-exec-47
=== 2015-05-20 15:29:33,493 [c-47] DEBUG PoolableDataSourceFactory - Created DataSource 170 of type 'List' in the pooling flow
=== 2015-05-20 15:29:33,494 [c-47] DEBUG PoolableDataSourceFactory - Activated DataSource 170 of type 'List'
=== 2015-05-20 15:29:33,494 [c-47] DEBUG ISCKeyedObjectPool - Borrowing object for 'elem'
=== 2015-05-20 15:29:33,494 [c-47] DEBUG PoolableDataSourceFactory - Created DataSource null of type 'elem' in the pooling flow
=== 2015-05-20 15:29:33,496 [c-47] DEBUG RPCManager - Processing 1 requests.
=== 2015-05-20 15:29:33,496 [c-47] DEBUG ISCKeyedObjectPool - Borrowing object for 'animals'
=== 2015-05-20 15:29:33,496 [c-47] DEBUG PoolableDataSourceFactory - Activated DataSource 101 of type 'animals'
=== 2015-05-20 15:29:33,496 [c-47] DEBUG DSRequest - Caching instance 101 of DS 'animals' from DSRequest.getDataSource()
=== 2015-05-20 15:29:33,496 [c-47] DEBUG DSRequest - Caching instance 101 of DS animals
=== 2015-05-20 15:29:33,497 [c-47] DEBUG RPCManager - Request #1 (DSRequest) payload: {
    criteria:{
        scientificName:"Allligator mississippiensis2"
    },
    values:{
        picture:"Alligator.jpg",
        information:"In the 16th century, Spanish explorers in what is now Florida encountered a large formidable animal which they called \"el largo\" meaning \"the lizard\". The name \"el largo\" gradually became pronounced \"alligator\".",
        commonName:"Alligator (American)",
        lifeSpan:50,
        scientificName:"Allligator mississippiensis2",
        diet:"Carnivore",
        status:"Not Endangered",
        _selection_23:true
    },
    operationConfig:{
        dataSource:"animals",
        repo:null,
        operationType:"update",
        textMatchStyle:"exact"
    },
    componentId:"isc_DynamicForm_0",
    appID:"builtinApplication",
    operation:"animals_update",
    oldValues:{
        picture:"Alligator.jpg",
        information:"In the 16th century, Spanish explorers in what is now Florida encountered a large formidable animal which they called \"el largo\" meaning \"the lizard\". The name \"el largo\" gradually became pronounced \"alligator\".",
        commonName:"Alligator (American)",
        lifeSpan:50,
        scientificName:"Allligator mississippiensis",
        diet:"Carnivore",
        status:"Not Endangered",
        _selection_23:true
    }
}
=== 2015-05-20 15:29:33,497 [c-47] INFO  IDACall - Performing 1 operation(s)
=== 2015-05-20 15:29:33,497 [c-47] DEBUG DeclarativeSecurity - Processing security checks for DataSource null, field null
=== 2015-05-20 15:29:33,497 [c-47] DEBUG DeclarativeSecurity - DataSource animals is not in the pre-checked list, processing...
=== 2015-05-20 15:29:33,498 [c-47] DEBUG AppBase - [builtinApplication.animals_update] No userTypes defined, allowing anyone access to all operations for this application
=== 2015-05-20 15:29:33,498 [c-47] DEBUG AppBase - [builtinApplication.animals_update] No public zero-argument method named '_animals_update' found, performing generic datasource operation
=== 2015-05-20 15:29:33,500 [c-47] INFO  SQLDataSource - [builtinApplication.animals_update] Performing update operation with
	criteria: {scientificName:"Allligator mississippiensis2"}	values: {picture:"Alligator.jpg",information:"In the 16th century, Spanish explorers in what is now Florida encountered a large formidable animal which they called \"el largo\" meaning \"the lizard\". The name \"el largo\" gradually became pronounced \"alligator\".",commonName:"Alligator (American)",lifeSpan:50,scientificName:"Allligator mississippiensis2",diet:"Carnivore",status:"Not Endangered",_selection_23:true}
=== 2015-05-20 15:29:33,500 [c-47] INFO  SQLValuesClause - [builtinApplication.animals_update] Ignored data for non-existent or included columns: [_selection_23]
=== 2015-05-20 15:29:33,503 [c-47] DEBUG PoolableSQLConnectionFactory - [builtinApplication.animals_update] DriverManager fetching connection for HSQLDB via jdbc url jdbc:hsqldb:hsql://localhost/isomorphic
=== 2015-05-20 15:29:33,503 [c-47] DEBUG PoolableSQLConnectionFactory - [builtinApplication.animals_update] Passing JDBC URL only to getConnection
=== 2015-05-20 15:29:33,607 [c-47] DEBUG PoolableSQLConnectionFactory - [builtinApplication.animals_update] makeObject() created an unpooled Connection '54777134'
=== 2015-05-20 15:29:33,607 [c-47] DEBUG SQLConnectionManager - [builtinApplication.animals_update] Borrowed connection '54777134'
=== 2015-05-20 15:29:33,607 [c-47] DEBUG SQLTransaction - [builtinApplication.animals_update] Started new HSQLDB transaction "54777134"
=== 2015-05-20 15:29:33,608 [c-47] DEBUG SQLDriver - [builtinApplication.animals_update] About to execute SQL update in 'HSQLDB' using connection'54777134'
=== 2015-05-20 15:29:33,608 [c-47] INFO  SQLDriver - [builtinApplication.animals_update] Executing SQL update on 'HSQLDB': UPDATE animals SET commonName='Alligator (American)', diet='Carnivore', information='In the 16th century, Spanish explorers in what is now Florida encountered a large formidable animal which they called "el largo" meaning "the lizard". The name "el largo" gradually became pronounced "alligator".', lifeSpan=50, picture='Alligator.jpg', status='Not Endangered' WHERE (LOWER(animals.scientificName)='allligator mississippiensis2')
=== 2015-05-20 15:29:33,610 [c-47] WARN  SQLDataSource - [builtinApplication.animals_update] update operation affected no rows
=== 2015-05-20 15:29:33,611 [c-47] DEBUG DSRequest - freeOnExecute is false for request of type update on DataSource animals - not freeing resources!
=== 2015-05-20 15:29:33,611 [c-47] DEBUG RPCManager - Content type for RPC transaction: text/plain; charset=UTF-8
=== 2015-05-20 15:29:33,611 [c-47] DEBUG SQLTransaction - Committing HSQLDB transaction "54777134"
=== 2015-05-20 15:29:33,612 [c-47] DEBUG RPCManager - non-DMI response, dropExtraFields: false
=== 2015-05-20 15:29:33,612 [c-47] DEBUG SQLTransaction - getConnection() found transactional connection for HSQLDB with hashcode "54777134"
=== 2015-05-20 15:29:33,612 [c-47] DEBUG SQLTransaction - Ending HSQLDB transaction "54777134"
=== 2015-05-20 15:29:33,613 [c-47] DEBUG SQLConnectionManager - About to close JDBCConnection with hashcode "54777134"
=== 2015-05-20 15:29:33,614 [c-47] DEBUG PoolableDataSourceFactory - Cleared and passivated DataSource 101 of type 'animals'
=== 2015-05-20 15:29:33,614 [c-47] INFO  Compression - /BuiltInDS/builtinds/sc/IDACall: 159 -> 140 bytes

Best regards
Blama

[Blama]

I just realized that this is not exactly the same issue as the Form does send an update-request.
For me it seemed like an add in the beginning because of the doubled rows.

Best regards
Blama

[Isomorphic]

Return again to the docs for getSaveOperationType() - the BuiltInDS code contains an explicit call to editRecord(), so that establishes a saveOperationType of "update".

So again no bug, but we may modify the sample to make this clearer.

[Blama]

Hi Isomorphic,

please re-read. I said that the fact that an update-operation is issued is correct. It is just that the update itself is wrong (or the serverside is wrong in its action). In the resulting SQL after the WHERE there is "WHERE pkField = newValue", obviously hitting 0 rows.

Best regards
Blama

[Isomorphic]

The client can initiate a DSRequest that will affect zero rows and the server will execute it faithfully, that's not a bug either.

[Blama]

Hi Isomorphic,

the client can do that, of course. But this specific form to edit an existing value should not.

Or to rephrase: How to set up a DynamicForm that can update the PK-value.
The DynamicForm.getSaveOperationType() docs say:

If saveOperationType is unset, the form will heuristically determine whether an "add" or "update" operation is intended based on whether the primaryKey field is present and editable.

This does only make sense if it is possible to update the displayed record, then.

Best regards
Blama

[Blama]

The easiest solution for the user here is not to have text-PKs (synthetic sequence PKs instead) and never show PKs to the user - then an edit is never required.
Updating a PK is anyway something at least strange to do, but if it is possible, it should work.

Best regards
Blama

[Isomorphic]

Again, please read the *next few sentences* of the same doc you've quoted twice now - it mentions that API calls like editNewRecord() will establish a saveOperationType.

As we mentioned, editRecord() is another example of such a call. The docs of editRecord() clearly say that it sets dynamicForm.saveOperationType to "update".

setValues() would be one way of populating the form without also establishing a saveOperationType.

However, as you correctly point out, this is a not something most people need to worry about: it's very rare for an application to allow editing of an existing PK. Where it is allowed, it's not going to be just an editRecord() and saveData() call, rather, it's going to require a "remove" followed by an "add", in a transaction.

[Blama]

Hi Isomorphic,

I understood the editRecord() part. I wanted to point out that the used heuristic of "PK present and editable" does not make sense if no successful (data changing) update is possible when the PK field indeed is editable.

Thanks for the "remove"+"add-explanation for if I want to edit a PK sometime (I'm a fan of synthetic PKs though and hope that I'll not need it).

With respect to the BuiltInDS-sample, that uses boundForm.editRecord(record): Perhaps the PK should be canEdit:false in order not to run into this when using the sample.
It is really confusing to see the rows double itself - but only until reload.

Best regards
Blama

[Isomorphic]

Again, yes, editing PKs rarely makes sense from an application perspective, and synthetic PKs are likewise what we recommend.

However when an application configures a PK field as editable, we don't presume to just disallow it entirely, as some systems do accommodate PK changes as part of an "update".

Our SQLDataSource does not, by design, since most SQL engines do not allow this or use special syntax for it.

As previously mentioned, we may update the sample code to avoid any confusion.

[Blama]

Ok, thanks for clarification.

Best regards
Blama