Closed zzh8829 closed 4 months ago
this seems affect the covariant test for some reason that might need your expertise
it appears that
func (v *ContentFieldsArticle) implementsGraphQLInterfaceNext() {}
func (v *ContentFieldsArticle) implementsGraphQLInterfaceRelated() {}
are extraneous for some unknown reason
but
func (v *CovariantInterfaceImplementationRandomItemContentRelatedVideo) implementsGraphQLInterfaceContentFields() {
}
correctly fixed another generation issue
this seems affect the covariant test for some reason that might need your expertise
it appears that
func (v *ContentFieldsArticle) implementsGraphQLInterfaceNext() {} func (v *ContentFieldsArticle) implementsGraphQLInterfaceRelated() {}
are extraneous for some unknown reason
but
func (v *CovariantInterfaceImplementationRandomItemContentRelatedVideo) implementsGraphQLInterfaceContentFields() { }
correctly fixed another generation issue
nvm i figured it out! small issue with embedded type checking
Hmm, I think I sort of understand what's going on here but I'm not sure if this is the right fix. Specifically, it seems a bit sketchy to me to be adding this as we add implementations -- couldn't we get duplicate methods or something? Shouldn't we be writing all implementations with the interface we are writing the implementations for?
I'm wondering if the actual problem is that, for example, TopicNewestContentNewestContentArticle
is somehow ending up without a field SimpleLeafContent
which would provide the method. Instead, it has only Typename
. We can add the method, but using that fragment won't work since the field isn't there. That's what happens elsewhere in the test case, I think -- the added methods should be redundant.
But I may be misunderstanding -- this fragment stuff gets super confusing and it's been a year or two since I last thought it through. It's not a huge problem to add redundant methods if there's a reason, I'm just worried this doesn't actually suffice to fix the problem with this query.
I think you are right I was mostly following the compile error. the real fix is we need to make TopicNewestContentNewestContentArticle embed SimpleLeafContent and unmarshal it correctly.
a good example of that would be InnerQueryFragmentRandomItemArticle the difference in implementation is that the previous nested marshalling only works for struct reference, it failed here because the embedded field is a fragment of a union.
okay i did some digging and found a better solution
so I added a special check for union type matching and it seems to solve the root cause
I love how simple the new solution is! I'll leave it to @benjaminjkraft to do the actual reviewing, since I don't understand all the subtleties here, but I'm wondering if you want to change the title of this PR given what ended up being the actual problem.
I have:
before the fix
after fix https://github.com/Khan/genqlient/pull/310/files#diff-3eaa9f1b68120a67e7558f37dd5984e995a6df5d454656180befca84c2dbf53aR2164
I also took a look at https://github.com/Khan/genqlient/issues/64 which is the real underlaying issue i wanted to fix but that seems to be extremely complicated and probably needs multiple weeks of work. the nested interface issues came up on a query with union spreading. this PR fixes the problem of spreading when the union is the same fragment, however if the spread contains different fragment we run into #64 .