Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Can not create new field which the same name with ingored field #1453

Closed
giangcoi48k opened this issue Feb 7, 2020 · 12 comments
Closed

Can not create new field which the same name with ingored field #1453

giangcoi48k opened this issue Feb 7, 2020 · 12 comments
Assignees
Milestone

Comments

@giangcoi48k
Copy link

When I created an ObjectType, I need Ignore some fields like:

descriptor
    .Ignore(t => t.FinancialStatements)
    .Ignore(t => t.Locations);

Then I need to create a new field FinancialStatements, like:

descriptor.Field("financialStatements")
  .Type<ListType<ObjectType<FinancialStatementModel>>>()
  .Argument("source", a => a.Type<ReportSourceType>())
  .Argument("isAudit", a => a.Type<BooleanType>())
  .Argument("isConsolidated", a => a.Type<BooleanType>())
  .Argument("isDirect", a => a.Type<BooleanType>())
  .AddResolver<FinancialStatementsResolver>();

But the new financialStatements field does not show, with error message: The field `financialStatements` does not exist on the type `CompanyReportType`.

This bug occurred on 10.3.5. In 10.2.0, this bug did not occur

@michaelstaib
Copy link
Member

Can you create a little repro?

@michaelstaib michaelstaib added the 🔍 investigate Indicates that an issue or pull request needs more information. label Feb 7, 2020
@michaelstaib michaelstaib added this to the HC-10.4.0 milestone Feb 7, 2020
@giangcoi48k
Copy link
Author

I created a project to demo this issue. Please take a look at this project: https://github.com/giangcoi48k/DemoIssue1453_Hotchocolate

@giangcoi48k
Copy link
Author

In the demo above, it seemed a bit ridiculous to ignore a field and then declare it immediately below. But imagine, if one class inherits another which ignored a few fields, and wants to re-enable that field. It's easy with version 10.2.0, but with version 10.3.5, I haven't found a way to do that yet

@michaelstaib
Copy link
Member

It is a valid case.... thanks for putting this together :)

@michaelstaib michaelstaib added bug and removed 🔍 investigate Indicates that an issue or pull request needs more information. labels Feb 7, 2020
@michaelstaib michaelstaib self-assigned this Feb 7, 2020
@PascalSenn
Copy link
Member

Hi @giangcoi48k
I'm pretty sure this field is still there :) I just think it is still ignored ;)
We added already a way to unignore to V11
#1458

@michaelstaib
Copy link
Member

michaelstaib commented Feb 7, 2020

@PascalSenn

The issue here is that we are also tracking the field name ... but in this case we only need to track the field property. But there are quite a few edge case hidden here .

Case 1

  1. Ignore field
  2. Unignore field
    => should just work

Case 2

  1. Ignore field
  2. Add new field with the field that we have ignored
  3. Unignore field that we ignored in one
    => The field ignored in 1 and unignored in 3 will become foo1

Case 3

  1. Add new field by name (foo)
  2. Add field by property binding (t => t.Foo)
    => The second field becomes foo1

@giangcoi48k
Copy link
Author

I think if we add a field by name (foo), but it shows as foo1 will be confusing. I think simply, if we declare a field below another field of the same name, the above field will be overwritten.

@PascalSenn
Copy link
Member

Hmm.. I also think it is confusing when we add another field. I would just always return the same instance of the field though
This way you can polymorphically extend the type.
If you add a new field you have to know how the parent was implemented to override fields. We would have to know about implementation details of the parent

@michaelstaib
Copy link
Member

The cases are a bit wrong 🤪 I will update them tonight the it will be more clear

@michaelstaib
Copy link
Member

So I have reworked the cases.

We could also decide to throw exceptions.

But, I think it can actually lead in useful cases like I might ignore a field in the main type.

Then later I extend the type and will include the ignored property under a different name.

I actually think that foo1 will never come to pass but we should have these cases covered.

@michaelstaib
Copy link
Member

#1468

@tobias-tengler
Copy link
Collaborator

Closing this, since it appears to be fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants