Parsing amounts incorrectly from string to decimal

[jgalvis-sw]

When the WebConnector's host OS is setup with a different culture (having a different decimal symbol) than the server running the QBSync.QbXml library, the values parsed in QbSync.QbXml.Objects.FLOATYPE from string to decimal are being parsed wrong. This is the case when QBSync.Qxml is running on en-US, where the decimal symbol is point (".") and the WebConnector is running on fr-CA, where the decimal symbol is comma (","). eg, an amount sent in the original QbXml by the WebConnector as "20,00", will be parsed to "2000.00".

Looking at the code, we can see that the private method Parse in QbSync.QbXml.Objects.FLOATYPE implementation, used to parse the values from string to decimal is using the Decimal.TryParse overload that receives only the string as parameter. This overload will use the current thread culture (in the case of a web app).

       private static decimal Parse(string value)
        {
            if (decimal.TryParse(value, out decimal output))
            {
                return output;
            }

            return 0m;
        }

This issue could be solved using the Decimal.TryParse overload that receives also the number style and the culture as parameter. It means that the culture should be sent a) as parameter on every call to deserialize a FLOATYPE, or b) sent as parameter in the FLOATYPE constructor and use it in the Parse method.

This is a little example to illustrate the problem and the solution:
https://dotnetfiddle.net/V20rXq

This issue is probably also impacting the parsing from decimal to string, however, after some tests I've made with the WebConnector, looks like it is able to manage properly both decimal symbols regardless the host culture.

In my opinion, it is important that the culture will be set explicitly for each usage of the serialization logic, or at the app initialization (or both), to highlight the fact that the culture used to serialize/deserialze the QbXmls actually matters, and may impact the values of some properties.

[tofer]

FLOATTYPE.Parse() is called by FLOATTYPE.ReadXML from the IXmlSerializable interface, so I'm not sure how we'd update it to support a specific culture on that level.

One option may be to use app.UseRequestLocalization(); to set the current context for each request. My local copy of QuickBooks does not send a Accept-Language header with it's requests, which is too bad (but perhaps check yours to be sure).

I did a quick test using a query string instead which seemed promising. I appended the AppUrl of WebConnectorQwcModel with the culture (ie AppUrl = "myurl.asmx?culture=fr-CA"). In your situation hopefully you could adjust the QWC file depending on the user's culture.

Update the startup something like this:

        public void Configure(IApplicationBuilder app, IHostingEnvironment env)
        {
            // ...

            var supportedCultures = new List<CultureInfo>
            {
                new CultureInfo("en-US"),
                new CultureInfo("fr-CA")
            };

            // Be sure to add localization before web connectcor
            app.UseRequestLocalization(x =>
            {
                x.DefaultRequestCulture = new RequestCulture("en-US");
                x.SupportedCultures = supportedCultures;
                x.SupportedUICultures = supportedCultures;
            });

            app.UseWebConnector(options =>
                {
                    options.SoapPath = "/QBConnectorAsync.asmx";
                });

            // ...
        }

[jsgoupil]

Quite the interesting finding here. Are you saying that QuickBooks is sending to your server 20,00 ? If that's the case, I think we should be able to replicate that by setting our desktop computer to a different region?
What you found @tofer, if it works, that's simply pure magic! A little too magic actually for any user to figure this out? I had never seen this. If your idea works, we should definitely add it to the README at least.

[jgalvis-sw]

@jsgoupil Yes, to replicate the scenario you can modify the Windows regional settings on the computer that is running the WebConnector, actually just changing the decimal symbol for the currencies from point to comma should be enough.

@tofer Unfortunately the solution you propose will work only if you know in advance what is the culture of the client you are going to integrate in order to build the URL and include it in the QWC file. However it won't be always the case, and also it will fail again if for some reason the client's decimal symbol changes after the initial setup.
Also, changing the whole application culture context may impact other things, like the language of the exception's messages for example, and if the application using QbSync.QbXml library is not only managing the communication with the WebConnector but performing other operations and parsings that involves the culture they may be impacted too, or may require to switch culture contexts many times on the fly.

I haven't look yet in detail the QbSync.QbXml implementation, but I'd say that probably the classes defined on QbSync.QbXml.Objects may require more logic when setting the value of the properties than just assigning the value, as ItemNonInventoryRet.EditSequence setter does.

I'll check the implementation closely to try to find/propose a solution...

[jsgoupil]

We generate some getters/setters for a lot of things in there. So we can most likely fix the problem, but before you venture there, do you have a way to find out which Culture the client is in? Is there a way to get it from a QbXml call? from the first request the client sends?
If not, we will have to change the qwc AppUrl like @tofer proposed.

PS. I'm on vacation right now, not allowed to code 🏒

[tofer]

I inspected the HTTP request and the body coming from WebConnector, and could not find anything that would indicate culture, only Country. Like @jsgoupil asks... how do you know what culture to use if not asking the user initially? Is there something I'm missing in the request?

I would still think the approach would be to set the CultureInfo.CurrentCulture, even if it wasn't by app.UseRequestLocalization();. We are dependent on Microsoft's XmlSerializer to parse the XML, so passing values in via constructors doesn't seem feasible. Perhaps extending IWebConnectorHandler to set the culture per request somehow? Again this comes back to how to determine what the right one is.

If deserialization is the only issue as you say (and serialization can stay decimal points), perhaps another solution would be to not worry about what the culture is, but to have parsing allow either comma or decimals. We would need to make sure WebConnector never sends commas for thousands place separators. If that was true, and this was possible, then we wouldn't need to worry about the cutlure, and we wouldn't need to pass anything into the deserialization process. I'll try to test if I can grab some time.

[tofer]

@jgalvis-sw I enabled multi-currency mode in QB, and changed my USD from period decimal separator to comma, and requests are still coming from the WebConnector with period decimal separators.

Seems like this might only be for display in QB. Do I have to change my system wide language to test this?

[jgalvis-sw]

@tofer Actually are the OS Regional settings what you have to change. The WebConnector always change the response coming from the SDK using the OS culture.

@jsgoupil @tofer To answer your question: Yes, actually I'm able to identify the culture "dynamically". In my implementation I perform some validations using the strHCPResponse sent in the first call, and performing some queries before execute the "real" job.
So from the HostQueryRs I'm getting the country, and I'm using a custom PrivateData attribute ( See QB programmer guide, chapter 11: Data Ext: Custom Fields and Private Data) binded to the QB Company entity to push decimal values, and then I compare them against the values received in the response to identify the decimal symbol. With this two elements I'm able to identify what is the culture of the WebConnector, specially for the multi-lang cultures like fr-CA and en-CA.

[tofer]

Ah, thanks @jgalvis-sw. So if I am understanding correctly, you are setting a DataExt field, in one request, then reading it back in the next request to determine the formatting of the decimal place?

This doesn't seem like something we can bake into the library easily. I just examined a request from WebConnector, and it for me at least I am not getting any thousands place separators in number values. What do you think of the option for allowing either commas or periods for decimal separators, and not worrying about the culture?

Change FLOATTYPE something like:

        public void ReadXml(System.Xml.XmlReader reader)
        {
            reader.MoveToContent();
            var isEmptyElement = reader.IsEmptyElement;
            reader.ReadStartElement();
            if (!isEmptyElement)
            {
                _value = Parse(reader.ReadContentAsString().Replace(',','.'));
                reader.ReadEndElement();
            }
        }

        private static decimal Parse(string value)
        {
            if (decimal.TryParse(value, NumberStyles.Number, CultureInfo.InvariantCulture, out decimal output))
            {
                return output;
            }

            return 0m;
        }

[jsgoupil]

We could bake it in the library with a options.DetectCulture() which would create a step. It's a little much though, don't you have to create your DataExt first, then push a value to "something"... It's been a while for me since I worked on this.

And, from your screenshot @jgalvis-sw , if you just change your decimal symbol, it doesn't change your culture from what I remember? So really just reading the country does not actually detect the comma, correct? So we would be stuck with making a dummy step.

I'm quite surprised the QbXml PDF does not mention this, or at least someone complaining on their forum?

[jgalvis-sw]

@tofer about thousands place separators: you're right, in all tests I made I didn't see any thousand group separator, however I didn't test exhaustively all the amount/quantity fields that could be returned in a QBXML neither all the cultures supported by Quickbooks.
About the solution to replace commas by points: I think it will force to set the culture of the application running theQBSync.QBXmlto a culture with point as decimal symbol. I mean, If my application is set to run with FR-CA it won't work, or I'd be force to switch the context culture before invoking any QbSync.QbXml feature.

@tofer @jsgoupil In my implementation I'm using the QbSync.QbXml nuget only , I'm no using the other projects.

About the DataExt field, yes is like @jsgoupil said, you have to create the field first and then push the value, however I play with the fact that the WebConnector executes all the queries contained in a single Qbxml request sequentially...So that in the same QbXml request, with onError="Continue" I send the query to add the custom DataExt field first and then the query to modify the value of that field. If the field already exists the first query will fail, but the second query will be executed anyway. In the response I only check the response of the second query, to get the value and identify if the decimal symbol has changed.

@jsgoupil About changing only the decimal symbol vs the whole culture:
I opened a support ticket with Intuit about that, and they pointed me out that it is a requirement that the Quickbooks host's regional settings match with the Quickbooks Country-version. So, to reproduce the issue we can just modify the OS decimal symbol, but it would be rare having a client with this setup in the real life as Quickbooks won't work properly.

I still think that the cleaner solution is to have in count the culture, and -just speculating- if you think about it probably other fields, like DATETYPE may be impacted too, as the date formats also change depending on the culture, eg mm/dd/yyyy in EN-US vs dd/mm/yyyy in EN-CA. Again, I'm just speculating about the dates because I've not worked with dates in my implementation.

[tofer]

About the solution to replace commas by points: I think it will force to set the culture of the application running theQBSync.QBXmlto a culture with point as decimal symbol. I mean, If my application is set to run with FR-CA it won't work, or I'd be force to switch the context culture before invoking any QbSync.QbXml feature.

I had already thought of this actually. If you notice in my code change suggestion, I forced the Decimal.TryParse in FLOATTYPE to use the InvariantCulture. This way it ignores the current culture of your hosting environment and will always use the Invariant, which will use periods for the decimal place. If your application runs with fr-CA, that should be fine.

Regarding a possible issue with culture and DATETYPE, I am also -just speculating- here, but would guess these will be unaffected. IIRC, the documentation says it will use format yyyy-MM-dd. Additionally, my regional computer settings show m/d/yyyy, yet it still comes in yyyy-MM-dd.

I would agree about factoring in the culture as the cleaner solution, however in your method of checking you still don't actually know the culture, you are just guessing based on the country and how the numbers or formatted.

[jgalvis-sw]

@tofer You're right! Sorry I missed the Invariant culture. As there're no others parsing impacted by the culture and we haven't find yet an accurate method to identify the client culture, I think what you propose is enough to solve this issue!

[jsgoupil]

I would highly value not using a hack, before we jump into implementation, we should make sure we are on the same page. I'll have a look later when I'm allowed to use quickbooks as well.
https://en.m.wikipedia.org/wiki/Decimal_separator

[jsgoupil]

After investigation, I am not getting that we are on en-US or fr-CA. But I am able to see that more than FLOATTYPE is affected.

On this screenshot, we can see that the data coming on the first request also affects PERCENTTYPE and AMTTYPE. And most likely PRICETTYPE would be affected. Unsure about QUANTYPE, but it most likely be affected (to be verified), so the fix could be in FLOATTYPE.

I would not do a Replace(",", ".") trick, .NET supports parsing based on culture so we should definitely leverage that.

The solution that @jgalvis-sw proposed of creating a fake invoice and reading it is definitely not my favorite. Probably the most fullproof but quite the chatty messages with QuickBooks.
If it were to be implemented, it can be implemented as a step.

services
    .AddWebConnector(options =>
    {
        options
            .AddAuthenticator<QuickBooksAuthenticator>()
            .WithWebConnectorHandler<WebConnectorHandler>()
            .WithCultureDetection() // New

I think we would need to save the culture alongside the ticket/step.

The other way to fix the problem is the idea @tofer proposed at the beginning and to get the Culture on the web connector connection.
In this case, we would change the implementation of GetQwcFile() and add a culture to the WebConnectorQwcModel.
We would then append a ?|&culture=XX-YY to the URL to finally detect it when the connector is connecting.
Now that's another complicated matter. The .NET Core is using SoapCore.
Which uses AddSoapServiceOperationTuner to capture the httpRequest. (not tested here...), but it seems we would be able to gather the query string.
That is not available in the version 0.9.9. Most likely available in 1.0
With this solution, I think we don't have to save the culture in the database since we have it at every request.

Since these two options are opt-in, I am up for any/both. But I don't have the time to implement it at the moment.

[tofer]

My $0.02 is that doing culture detection via steps should not be the way to go:

A) It's complicated
B) You can't actually detect the culture, only formatting of numbers. You would have to know all the cultures of the world, and which multi-lang cultures had what formatting for each language. This just doesn't seem plausible library wide.

Adding an option to specify culture on an account (or basically during Qwc download) seems fine with me, since the user can then actually select their culture. This may solve other user's issues, but doesn't solve for @jgalvis-sw .

I know you don't like the comma replacement, but we could probably make that opt-in as well. It (or something similar for 'multi-culture leniency') is still my vote ;)

[tofer]

Is FinanceChargePreferences always included in strHCPResponse? If so what's preventing us from reading if there is a comma or a period in AnnualInterestRate or MinFinanceCharge right out of the gate, avoiding a DataExt write/read step?

Also, would creating a custom culture feel less hacky than doing a string replace? What about something like this in FLOATTYPE:

NOTE: Untested

        private static decimal Parse(string value)
        {
            var culture = DetectCulture(value);

            if (decimal.TryParse(value, NumberStyles.Number, culture, out decimal output))
            {
                return output;
            }

            return 0m;
        }

        private static CultureInfo DetectCulture(string value)
        {
            // Could we determine if culture detection is enabled or not somehow here?

            if (string.IsNullOrWhiteSpace(value)) return CultureInfo.InvariantCulture;

            return value.LastIndexOf(',') > value.LastIndexOf('.') 
                ? DecimalCommaCulture 
                : CultureInfo.InvariantCulture;
        }

        /// <summary>
        /// A customization of the Invariant culture that uses a comma decimal separator
        /// </summary>
        private static readonly CultureInfo DecimalCommaCulture = new CultureInfo(string.Empty)
        {
            NumberFormat =
            {
                NumberDecimalSeparator = ",",
                NumberGroupSeparator = ".",
                CurrencyDecimalSeparator = ",",
                CurrencyGroupSeparator = ".",
                PercentDecimalSeparator = ",",
                PercentGroupSeparator = "."
            }
        };

[jsgoupil]

@tofer that's hacky to me :(
I'm asking @williamlorfing on the Intuit website.
https://help.developer.intuit.com/s/question/0D54R00006oDV27SAG/decimal-comma-vs-decimal-point-in-qbxml?t=1581623494141

I met him once at a QuickBooks convention, let's see what he has to say on this.

[jsgoupil]

So William responded on the QuickBooks thread, and he doesn't have any recommendations.
I'm voting for the query string sniffing.