graphql-platform icon indicating copy to clipboard operation
graphql-platform copied to clipboard

Specified fields order does not preserved for methods

Open bohdan-kolomiiets opened this issue 3 years ago • 2 comments
trafficstars

Is there an existing issue for this?

  • [X] I have searched the existing issues

Describe the bug

In annotation-based approach (at least tested with this), object methods are rendered always above properties, even though these methods specified below properties in c# code. And I haven't found a way to influence that (maybe with some attrbite at least).

I was expecting that class memebers order is preserved when they are transformed into object fields and rendered in graphql syntax.

I think that by specifing fields in certain order, engineer put some sense in it, making focus or putting attention on some fields more and on others less. Otherwise, for example in my real case, object schema results to have id, name fields somewhere in the middle which are logically expeted to be somewhere at the beginning because these fields may visually help to distinct objects by humans.

Steps to reproduce

Code

var builder = WebApplication.CreateBuilder(args);
builder.Services.AddGraphQLServer().AddQueryType<Query>();

var app = builder.Build();
app.MapGraphQL();
app.Run();

public class Query
{
    public SomeEntity GetEntity() => new SomeEntity();
}

public class SomeEntity
{
    [GraphQLType(typeof(IdType))]
    public string Id { get; set; }
    public string Ccc { get; set; }
    public string Bbb { get; set; }
    public string Aaa { get; set; }
    public string Z() => "123";
    public string A() => "123";
}

Actual object schema

... 

type SomeEntity {
  z: String!
  a: String!
  id: ID
  ccc: String!
  bbb: String!
  aaa: String!
}

...

Expected object schema

...

type SomeEntity {
  id: ID
  ccc: String!
  bbb: String!
  aaa: String!
  z: String!
  a: String!
}

...

PS: Applying GraphQLName attribute does influence generated fields order.

Relevant log output

No response

Additional Context?

No response

Product

Hot Chocolate

Version

12.13.2

bohdan-kolomiiets avatar Sep 10 '22 18:09 bohdan-kolomiiets

We can't order them the way you wrote them, since the underlying reflection API does not guarantee an order: https://docs.microsoft.com/en-us/dotnet/api/system.type.getmethods?view=net-6.0 We can order alphabetically, via ModifyOptions(options => { options.SortFieldsByName = true; }) (since this is just a sort on all the methods we got), but we can't guarantee a field order without an attribute or another indicator of order (like the fluent API). We could introduce something like an Order attribute though, but I'm not sure...

tobias-tengler avatar Sep 10 '22 19:09 tobias-tengler

Yes, I think some sort of Order attribute using which engineer would specify desired order could be a solution.

Also, I would suggest to put all fields that are generated from methods after/below those generated from properties, not before/above as it is now, by default, if engineer does not specified any custom attribute to influence order. I think, this behaviour can be implemented separatedly from Order attribute, and perhaps easier then Order attibute.

bohdan-kolomiiets avatar Sep 10 '22 22:09 bohdan-kolomiiets

Some kind of Order annotation may lead to even more confusion, e.g.:

class User {
  [Order(1)]
  string Id
  string Foo
  [Order(1)]
  string Name
  string Bar
  int Acme
}

Also, there is one more thing - no matter how we will sort the introspection query result, UI can render them in its manner (e.g. I can imagine that some UI may decide to render items in alphabetical order, so it won't matter if the backend will preserve order or not 🤷‍♂️)

It's like a tradeoff for the "number of constructor arguments" static code analysis check, e.g. it is bad to have a constructor with a quadrillion of boolean arguments, and probably it is true, then the question is it good to have an entity with a quadrillion of fields majority of which are intended as nonimportant? Wouldn't it be a better decision to have something like:

type User {
  id
  name
  metadata {
    foo
    bar
    acme
  }
}

Yes, yes, that that's is one more class and resolvers and so on, but at the very end it's not graphql related, aka the same is true for rest api (aka swashbuclke uses the same reflection with same restrictions)

mac2000 avatar Oct 20 '22 19:10 mac2000

We have alphabetic order ... the rest we cannot guarantee. I find working with order attributes not really a solution and would end up in chaos. If you want order ... just opt into alphabetic order.

michaelstaib avatar Jun 28 '24 17:06 michaelstaib