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

OrchardCore.Lists have GraphQL that implements ContainedPart, but the interface is not, is it a bug? #4603

Open
chinasqzl opened this issue Oct 22, 2019 · 15 comments · May be fixed by #17382
Open
Labels
Milestone

Comments

@chinasqzl
Copy link

image

image

@jrestall
Copy link
Contributor

I'm sorry, I don't understand the question, likely because I've never used a contained part or list before.

Maybe someone who has can offer their insight on whether the graphql for lists/contained part is what is expected.

@chinasqzl
Copy link
Author

chinasqzl commented Oct 23, 2019

@jrestall
ContainedPart is implicitly attached to the content type, and after I explicitly added it in ContentDefinition.json, GraphQL has it.

    {
          "PartName": "ContainedPart",
          "Name": "ContainedPart",
          "Settings": {
          }
        }

@sebastienros sebastienros added this to the 1.0.x milestone Oct 24, 2019
@carlwoodhouse
Copy link
Member

So I think basically your saying those properties appear to be missing off blogpost as presumably blog/blogpost use the containedpart ?

Honestly I dunno why the graphql implementation would only return the id anyway and not take you up the graph to the contentitem ..

I can take a look if you like @jrestall

@carlwoodhouse
Copy link
Member

Never used the part / lists before either but something does seem wrong

@deanmarcussen
Copy link
Member

The problem is that the ContainedPart is not part of the content type definition. It gets welded to the ContentItem when an blogpost is added to the blog list.

It's part of the idiosyncratic nature of lists, that they can contain any kind of content type, without having to add a contained part manually (like in O1)

But welded content parts, don't get handler calls, or in this case GraphQL calls.
In the ContentTypeQuery it only loops known content types.

Which is why when @chinasqzl you explicitly add it to the ContentDefinition.json it then starts showing up in the GraphQL query result.

@carlwoodhouse
Copy link
Member

urg ;/ Given that schema is generated once based off contentdefinition im not sure how we can fix that; other then always exposing the property on the interface but returning null if it's not welded

@carlwoodhouse
Copy link
Member

its still should probably be returning a ContentItemInterface not the id though for that property ;) so you can go up the graph.

@deanmarcussen
Copy link
Member

yeah, it's dificult with welded parts.

not just an issue with graphql but also mentioned here #3890

Basically you need to be going through the parent to get the list items. Not trying to do it the other way round.

But (and I don't know Graphql so much, so just looking at ideas)

FlowMetadataContentTypeBuilder : IContentTypeBuilder is also a welded part.

Does that work currently?

@jrestall
Copy link
Contributor

jrestall commented Nov 5, 2019

Yes that works and I also used a similar approach for menu items e.g. MenuItemContentTypeBuilder.

It works because a Widget always has a FlowMetadata part and a menu always has a MenuItemsListPart, but I'm struggling to see how we could determine if a content type should have a contained field.

Maybe a custom ContainedPartContentTypeBuilder that gets all content types that have a ListPart and then gets from their settings the names of all the content types that they can list. Then if it sees any of those Content Types during schema build, it will add a ContainedPart field dynamically. Would that make sense?

@deanmarcussen
Copy link
Member

Then if it sees any of those Content Types during schema build, it will add a ContainedPart field dynamically. Would that make sense?

Yeah, that would probably work. sounds pretty hacky though ;)

Other question. Does it have that much value to be able to get at the parent's id, when you would normally just go through the parent to get the content items. I don't know, is all I'm saying.

@jrestall
Copy link
Contributor

jrestall commented Nov 5, 2019

Other question. Does it have that much value to be able to get at the parent's id, when you would normally just go through the parent to get the content items. I don't know, is all I'm saying.

Yea hence I'm not committing to fixing this one myself, just offering up hacky advice for anyone who really feels like this is needed and wants to submit a PR, to me it's not a high priority though.

@carlwoodhouse
Copy link
Member

carlwoodhouse commented Nov 5, 2019 via email

@carlwoodhouse
Copy link
Member

eg.

someContentType {
  displayText
  description
  contained {
     contentItem {
       displayText
       url
   
       contains ......
    }
  }
}

@sebastienros
Copy link
Member

I'd suggest to add a custom query to filter list items by a containerid, but I feel like it was already done.

@jeffolmstead
Copy link
Contributor

If I could throw out a possible middle ground, would it be possible to just make the "ContainedPart" attachable? I have some content types that will only exist to be contained in a list under another content type so attaching the contained part (manually) for purposes of GraphQL would meet my needs without having to delve into ContentDefinition.json (or worse, when it is stored in a database). Then we can rely on documentation for the "edge" cases of when users (like me) want to see the listcontentitemid alongside of the content type.

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

Successfully merging a pull request may close this issue.

7 participants