ServiceStack / Issues

Issue Tracker for the commercial versions of ServiceStack
11 stars 6 forks source link

Dart client corrupts any dto's List properties in flutter web release mode (minification), and a suggested fix #756

Closed rockgecko-development closed 3 years ago

rockgecko-development commented 3 years ago

Say we have a API that returns a Foo:

class Foo implements IConvertible {
List<Bar> bars;
}
class Bar implements IConvertible {
String name;
String description
}

Rendering a list of Bars showing name and description works fine in dev mode, but in release mode where minification is applied, trying to read the name field throws:

NoSuchMethodError: method not found: 'toString' (a.toString is not a function)
js_primitives.dart:47     at Q.j (http://localhost:55276/main.dart.js:40912:24)
...
js_primitives.dart:47 Another exception was thrown: Instance of 'minified:ie<void>'

I spent a few hours digging and have the reason, and the fix:

It occurs due to reliance on runtimeType in SS's Json converters. We had previous issues with app obfuscation replacing List's runtime type with _GrowableList. Web minification actually changes the runtimeType of all classes!

Flutter advises not to ever use runtimeType.toString() here, and in my previous issue on this topic #721. I raised this issue with the Flutter team: https://github.com/dart-lang/language/issues/1056 but the conclusion is the same: we can't use it.

But! ServiceStack's json deserializer doesn't actually need to use runtimeType.

Option 1:

Change ListConverter to use the passed-in context.typeName, rather than the runtime type: https://github.com/ServiceStack/servicestack-dart/blob/73df179e899cefe28765f3c14b934f5ab83ae63a/lib/json_converters.dart#L329 var genericArgs = getGenericArgs(getTypeName(o)); => var genericArgs = getGenericArgs(context.typeName ?? getTypeName(o)); This works for my specific problem, but you'll probably need to do the same for MapConverter etc and run your unit tests. You could then probably completely remove this part https://github.com/ServiceStack/servicestack-dart/blob/73df179e899cefe28765f3c14b934f5ab83ae63a/lib/interfaces.dart#L69 where you add the runtime types of all generic types to TypeContext.types.

Option 2:

Add the runtimeType of every dto class to the TypeContext.types map, not only generic classes: just delete this where line: https://github.com/ServiceStack/servicestack-dart/blob/73df179e899cefe28765f3c14b934f5ab83ae63a/lib/interfaces.dart#L69

...and a proper fix for #721

I admit I led you astray by suggesting the runtimeType of a List<T> was always "List"! The original problem was actually, for an API that returns a raw List<Bar>, there was no corresponding TypeToken included at the bottom of the dto.dart, for the List, just one for Bar. The correct fix is for this to be included in the codegen server-side. But here's a workaround for JsonWebClient and JsonServiceClient : https://github.com/ServiceStack/servicestack-dart/blob/73df179e899cefe28765f3c14b934f5ab83ae63a/lib/web_client.dart#L505

} else if (info.request is List) {
...
}
String addedTypeKey;
        if (responseAs is List &&
            reqContext.getTypeInfo(_getListType(responseAs)) == null) {         

          //List response type was not included in the TypeContext codegen :(
          //Add it in
          //This workaround is not ideal, as it maintains a reference to the
          //first request object in order to call it's createResponse()
          var responseListType = _getListType(responseAs);

          reqContext.types[responseListType] = TypeInfo(TypeOf.Class,
              create: () => info.request.createResponse());
          addedTypeKey = responseListType;
        }

        fromMap = responseAs is List
            ? (dynamic _) => ListConverter().fromJson(jsonObj, reqContext)
            : responseAs.fromMap as Function;

        dynamic ret = fromMap(jsonObj);
        if (addedTypeKey != null) reqContext.types.remove(addedTypeKey);
        return ret as T;
      } on Exception catch (e) {
        raiseError(res, e);
        return jsonObj as T;
      }
    }
    ...
 }

    ///The runtimeType of a list in a release build is _GrowableList, not List !
String _getListType(List p) {
    var typeStr = p.runtimeType.toString(); //still not ideal, and won't work if it's a nested List<List<T>>...
    return "List<${typeStr.split("<").sublist(1).join()}";
  }

This workaround isn't ideal, as

The preferred fix is to include the List type in the generated dto server-side.

mythz commented 3 years ago

What's the easiest way to get a repro of this issue? i.e. what's the original C# DTOs and what client Dart code can I run to repro this?

rockgecko-development commented 3 years ago

Define these in C#:

public class Foo
    {
        public List<Bar> bars { get; set; }
    }
    public class Bar
    {
        public string name { get; set; }
        public string description { get; set; }
    }
    public class Baz
    {
        public string name { get; set; }
        public string description { get; set; }
    }
    public class FooRequest : IReturn<Foo> { }
    public class RawBazRequest : IReturn<List<Baz>> { }

public class IssueService : Service{
public Foo Get(FooRequest dto)
        {
            return new Foo
            {
                bars = new List<Bar> {
                    new Bar {
                        name = "bar item 1",
                        description = "item 1 description"
                    },
                    new Bar {
                        name = "bar item 2",
                        description = "item 2 description"
                    }
                }
            };
        }

        public List<Baz> Get(RawBazRequest dto)
        {
            return new List<Baz> {
                new Baz {
                    name = "item 1",
                    description = "item 1 description"
                },
                new Baz {
                    name = "item 2",
                    description = "item 2 description"
                }
            };
        }
}

For this issue: In flutter, call FooRequest and render the items in a list. Once working, run a web release build eg flutter run web --release, and it should crash when reading item.name.

For #721 : Observe that the dart codegen includes this TypeContext:

'Bar': new TypeInfo(TypeOf.Class, create:() => new Bar()),
    'Baz': new TypeInfo(TypeOf.Class, create:() => new Baz()),
'Foo': new TypeInfo(TypeOf.Class, create:() => new Foo()),
    'List<Bar>': new TypeInfo(TypeOf.Class, create:() => new List<Bar>()),

The List<Baz> entry is missing. List entries seem to be only included in the codegen if they are properties of another response. So you will see List, as it's a property of the Foo response.

In Flutter, call RawBazRequest and render the items in a list. I think this will fail on web or Android/iOS in debug too (I'm not sure, because I already have my workarounds). Build a release version of Android and web and try the same thing. This definitely fails.

rockgecko-development commented 3 years ago

Then try the same with Dictionary responses, and generic class responses eg

public abstract class AbsPagingResponse<T>{
    public int TotalCount { get; set; }
    public bool HasMore { get; set; }
    public List<T> Items { get; set; }
    }
    public class PagingResponse : AbsPagingResponse<Foo>{
        public string FooInfo { get; set; }
    }

These also don't work quite correctly due to problems in the codegen. But I don't specifically need them for my use case.

mythz commented 3 years ago

This should be resolved using the latest v5.10.5 of ServiceStack on MyGet and upgrading servicestack Dart client to the latest ^1.0.27.

rockgecko-development commented 3 years ago

Thanks for the fast turnaround. the .NET changes look good, and #721 is fixed on flutter app release builds, but unfortunately, neither issue is fixed on flutter web release. I get the same error. It seems ListConverter still calls getTypeName, which still equates to calling context.typeInfo.createInstance().runtimeType.toString() and passing that as the typeName arg to convert(), rather than using the context.typeName passed to ListConverter.

Making the same change in ListConverter: var genericArgs = getGenericArgs(getTypeName(o)); to var genericArgs = getGenericArgs(context.typeName ?? getTypeName(o)); fixes both issues for me (but again, I'm not sure of the consequences for nested lists or Maps etc).

C# Codegen Version: 5.105 pubspec: servicestack: 1.0.27

flutter doctor:

Doctor summary (to see all details, run flutter doctor -v):
[√] Flutter (Channel beta, 1.25.0-8.3.pre, on Microsoft Windows [Version 10.0.19041.804], locale en-AU)
[√] Android toolchain - develop for Android devices (Android SDK version 29.0.2)
[√] Chrome - develop for the web
[√] Android Studio (version 3.6)
[√] VS Code, 64-bit edition (version 1.42.0)
[√] Connected device (4 available)
mythz commented 3 years ago

Then I'm not able to repro the issue. I'm running with the latest stable version of Flutter using their default app template:

$ flutter create myapp

Then add our integration test dtos which now includes both your FooRequest and RawBazRequest Request DTOs:

$ x dart http://test.servicestack.net

Which I'm able to call without issue, i.e:

import 'package:servicestack/web_client.dart' if (dart.library.io) 'package:servicestack/client.dart';

IServiceClient client = ClientFactory.create("http://test.servicestack.net");
var result = await client.get(FooRequest());
var result = await client.get(RawBazRequest());

That I was able to test in release mode in Chrome with:

$ flutter run -d chrome --release --web-port 8080

If you're not using the latest stable build please switch to that, otherwise I'll need a repro I can run locally in order to identify & resolve issues.

rockgecko-development commented 3 years ago

Did you use/render the result of the API call? The error doesn't actually surface until you try to access a property of a element of a list in a response. Try this for example:

var result = await client.get(FooRequest());
print('result length: ${result.length}'); //this is fine. And if Foo had other properties, they would be ok to read here too.
print('result type (maybe minified): ${result.runtimeType} expected ${Foo().runtimeType}');
print('first bar type (maybe minified): ${result.first.runtimeType} expected: ${Bar().runtimeType}');
print('first bar.name property (crash!!): ${result.first.name}'); //crashes here when run on flutter web --release
print('first bar.description property (crash!!): ${result.first.description}');
mythz commented 3 years ago

Did you use/render the result of the API call?

Yeah I was serializing the deserialized DTO to JSON and displaying that, I've now added these object accessors:

String json = "";
List<String> sb = [];
try {
  sb.add("FooRequest():");
  var result = await client.get(FooRequest());
  sb.add(jsonEncode(result));
  sb.add('  result length: ${result.bars.length}');
  sb.add('  result type (maybe minified): ${result.runtimeType}');
  sb.add('  first bar type (maybe minified): ${result.bars.first.runtimeType}');
  sb.add('  first bar.name property: ${result.bars.first.name}');
  sb.add('  first bar.description property: ${result.bars.first.description}');
  sb.add('  bar.name properties: ${result.bars.map((x) => x.name).join(", ")}');
} catch (e, stackTrace) {
  sb.add('Error: $e\n$stackTrace');
}
setState(() {
  fooRequest = json;
});
try {
  sb.add("");
  sb.add("RawBazRequest():");
  var result = await client.get(RawBazRequest());
  sb.add(jsonEncode(result));
  sb.add('  result length: ${result.length}');
  sb.add('  result type (maybe minified): ${result.runtimeType}');
  sb.add('  first bar type (maybe minified): ${result.first.runtimeType}');
  sb.add('  first bar.name property: ${result.first.name}');
  sb.add('  first bar.description property: ${result.first.description}');
  sb.add('  bar.name properties: ${result.map((x) => x.name).join(", ")}');
} catch (e, stackTrace) {
  sb.add('Error: $e\n$stackTrace');
}

Which in a Chrome release build now results in: image

Using the latest v1.0.29 of servicestack dart library now on pub.dev and the latest ServiceStack v5.10.5 now on MyGet.

To mitigate minification issues ServiceStack now generates type names for all DTOs, it's also maintaining a reverse lookup of minified types that can be disabled with:

TypeContext.EnableMinifiedTypes = false;

Where getTypeName() would then need to fallback to use context.typeName.

I've also added a new Log provider and debug logging throughout the code base to make it easier to debug release builds. By default it's restricted to Warn and Error log levels, but you can turn on debugging logging in your App with:

void main() {
  Log.levels.add(LogLevel.Debug);
  runApp(MyApp());
}

Or disable all logging with either:

Log.levels.clear(); //or
Log.logger = new NullLogger();

Please let me know if the latest releases resolves your issue.

rockgecko-development commented 3 years ago

Thanks! This fixes it. Tested both dev and release builds of web and Android, all issues fixed. It works on release web now even with TypeContext.EnableMinifiedTypes = false;

mythz commented 3 years ago

Great, it's enabled by default because I believe resolving the minified type name from a reverse lookup is safer then relying on the heuristic of using the context.typeName which anyone creating child contexts wont know about. I've removed using getTypeName() from resolving a generic Map's Key/Value Types so that it's only used when resolving top-level types so the fallback should also be ok for the built-in usages of it.