Azure / azure-sdk-for-sap-odata

This repository serves as foundation to power SAP OData SDK development for .NET
MIT License
7 stars 3 forks source link

[BUG] UpdateAsync sends whole business partner instead of changed field only causing XML errors [xfer of #18] #11

Open WillEastbury opened 1 year ago

WillEastbury commented 1 year ago

Is there an existing issue for this?

Type of issue

bug

Describe the bug

var businessPartner = await bps.GetAsync("0100000001"); businessPartner.Address.Building = "16 Brook"; await bps.UpdateAsync(businessPartner);

translates to:

Expected Behavior

{ "Address" : { "City" : "Berlin", "PostalCode" : "13467", "Street" : "Calvinstra\u00DFe", "Building" : "16 Brook", "Country" : "DE", "AddressType" : "02", "metadata" : { "type" : "GWSAMPLE_BASIC.CT_Address" } }, "BusinessPartnerID" : "0100000001", "CompanyName" : "Becker Berlin", "WebAddress" : "http://www.beckerberlin.de", "EmailAddress" : "dagmar.schulze@beckerberlin.de", "PhoneNumber" : "1373", "FaxNumber" : "3088004", "LegalForm" : "GmbH", "CurrencyCode" : "EUR", "BusinessPartnerRole" : "02", "CreatedAt" : "/Date(1658152863000)/", "ChangedAt" : "/Date(1671638524230)/", "ToSalesOrders" : { "deferred" : { "uri" : "https://demo-sap-apim.azure-api.net:443/sap/opu/odata/iwbep/GWSAMPLE_BASIC/BusinessPartnerSet(\u00270100000001\u0027)/ToSalesOrders" } }, "ToContacts" : { "deferred" : { "uri" : "https://demo-sap-apim.azure-api.net:443/sap/opu/odata/iwbep/GWSAMPLE_BASIC/BusinessPartnerSet(\u00270100000001\u0027)/ToContacts" } }, "ToProducts" : { "__deferred" : { "uri" : "https://demo-sap-apim.azure-api.net:443/sap/opu/odata/iwbep/GWSAMPLE_BASIC/BusinessPartnerSet(\u00270100000001\u0027)/ToProducts" } }, "metadata" : { "id" : "https://demo-sap-apim.azure-api.net:443/sap/opu/odata/iwbep/GWSAMPLE_BASIC/BusinessPartnerSet(\u00270100000001\u0027)", "uri" : "https://demo-sap-apim.azure-api.net:443/sap/opu/odata/iwbep/GWSAMPLE_BASIC/BusinessPartnerSet(\u00270100000001\u0027)", "type" : "GWSAMPLE_BASIC.BusinessPartner" } }

instead of:

{ "Address": { "Building":"16" } }

SAP Gateway throws HTTP 500: Method 'IF_SXML_READER~GET_ATTRIBUTE_VALUE' cannot be called at this position in the XML stream

Steps To Reproduce

var businessPartner = await bps.GetAsync("0100000001"); businessPartner.Address.Building = "16 Brook"; await bps.UpdateAsync(businessPartner);

Add screenshots to help explain your problem

No response

Additional context

No response

WillEastbury commented 1 year ago

Triaged. We should

A) Efficiency -> Implement the unit of work pattern to track / compare the last loaded context data and only send back changed data.

B) Diagnostics on why this doesn't work with a full payload from the SAP Side.

WillEastbury commented 1 year ago

B) First, then A) As an Enhancement later.

WillEastbury commented 1 year ago

B) errors with an SAP-Side data error. I've again added a test function that updates and renders this object and it clearly throws.

Proceeding with A)

WillEastbury commented 1 year ago

OK, we can go old-school with an INotifyPropertyChanged type implementation here ?

Something like the below should give us what we need for the change tracking for PATCH operations.

    public List<string> ChangeLog {get; set;} = new List<string>();
    public event PropertyChangedEventHandler PropertyChanged;
    private void NotifyPropertyChanged([CallerMemberName] String propertyName = "")  
    {  
        PropertyChanged?.Invoke(this, new PropertyChangedEventArgs(propertyName));
    }  

    public void AddToChangeLog(string PropertyName, object Value)
    {
        ChangeLog.Add(PropertyName);
    }

    public void ClearChangeLog()
    {
        ChangeLog.Clear();
    }
    public Dictionary<string, object> GetChangeLog()
    {
        return ChangeLog.ToDictionary(x => x, x => this.GetType().GetProperty(x).GetValue(this, null));
    }

    public string GetChangeLogAsJSON()
    {
        // Render the ChangeLog AS JSON
        return JsonSerializer.Serialize(GetChangeLog());
    }
WillEastbury commented 1 year ago

We can formally implement INotifyPropertyChanged here too

This would also mean the objects in the SDK would be bindable in a number of other ways in desktop and web apps, which is probably not a bad thing

So

public abstract class BaseDTOWithIDAndETag : IBaseDTOWithIDAndETag, INotifyPropertyChanged

WillEastbury commented 1 year ago

@MartinPankraz this approach will not recognise row tracking (if you add a update a property on a child collection object - you'd have to call the update method on the child object. We might be able to address that with batching support later.

WillEastbury commented 1 year ago

How should we handle complex nested types (the CT_ pocos) in change tracking ? Should we send those as if they are part of the parent class?

WillEastbury commented 1 year ago

INotifyPropertyChanged, and IWorkTracking interfaces added to be able to track the state of the object, and accept or revert changes from the list.

Interface looks like this

public interface IWorkTracking : INotifyPropertyChanged { bool IsChanged {get;} void RevertChanges(); void AcceptChanges(); void UndoLastChange(); void UndoLastChange(string PropertyName);

Dictionary<string, object> GetChangeLog();
string GetChangeLogAsJSON();

}

WillEastbury commented 1 year ago

now object UndoLastChange and object UndoLastChange(string PropertyName) to get the values reverted

WillEastbury commented 1 year ago

You can also subscribe to this event that we now raise to track changes

PropertyChangedWithValuesEventArgs : PropertyChangedEventArgs