DML is NOT Recommended in APEX Property Setter

Question

I just got a strange recommendation from SalesForce support which was related to inability of APEX controller class to save value from VisualForce component to a custom setting.

The recommendation was literally "DML inside the property's getter and setter are not a good practice."

Here is what does not work:

...
    public String currentFunction {
        get {
            if (null == currentFunction) {
                return getCurrentFunction();
            }
            return currentFunction;
        }
        set {
            currentFunction = value;
            // these lines:
            TestSetting__c settings = TestSetting__c.getInstance();
            settings.Function__c = value;
            upsert settings;
        }
    }
...

The controller code above does not produce any exceptions, logs show the setting has been saved, but in reality it does not. I found nothing explaining that anywhere, including Apex Properties page on the dev guide.

I spent a month or so to solve this issue with the support, so i decided to share (partial!) solution for this case to give you a chance to not waste your time.

Update #1: as per Controller Methods dev guide,

It’s a best practice for setter methods to be idempotent, that is, to not have side effects. For example, don’t increment a variable, write a log message, or add a new record to the database. Visualforce doesn’t define the order in which setter methods are called, or how many times they might be called in the course of processing a request. Design your setter methods to produce the same outcome, whether they are called once or multiple times for a single page request.

IMO, that does not directly explain my case, but gives some useful recommendations. but my biggest concern is — why does that happen silently?

Answer 1

Solution for VisualForce

The solution is to do the DML in a separate method called by action call from the VF page/component.

Here is the controller class:

public with sharing class TmpController {
    public String noFilter { get { return 'No Filter'; } }
    public String currentFunction {
        get {
            if (null == currentFunction) {
                return getCurrentFunction();
            }
            return currentFunction;
        }
        set;
    }
    public SelectOption[] getPossibleFunctions() {
        return new SelectOption[]{
            new SelectOption('One', 'One'),
            new SelectOption('Two', 'Two'),
            new SelectOption('Three', 'Three')
        };
    }
    // here we do the DML
    public PageReference updateCSInfo() {
        TestSetting__c settings = TestSetting__c.getInstance();
        settings.Function__c = currentFunction;
        upsert settings;
        return null;
    }
    private String getCurrentFunction() {
        TestSetting__c settings = TestSetting__c.getInstance();
        return (null == settings.Function__c) ? noFilter : settings.Function__c;
    }
}

... and here is the VF page:

<apex:page controller="TmpController">
<apex:form>
<apex:pageBlock title="Tmp Page">
<apex:pageBlockSection>
<apex:pageBlockSectionItem >
<apex:outputLabel for="functionSelector" value="Show Stuff Only For:" />
<apex:selectList value="{!currentFunction}" title="Select a Function" size="1" id="functionSelector">
    <apex:actionSupport event="onchange" action="{!updateCSInfo}" />
    <apex:selectOption itemValue="{!noFilter}" itemLabel="No Filter" />
    <apex:selectOptions value="{!possibleFunctions}" />
</apex:selectList>
</apex:pageBlockSectionItem>
</apex:pageBlockSection>
</apex:pageBlock>
</apex:form>
</apex:page>

hopefully, that will be helpful.