Built in IPublishedContent properties can clash with user generated DocType aliases

[PeteDuncanson]

If you create a field with an alias of "name" on your doc type and then try to run the code it will blow up with an error about there being multiple "name" fields. So we are going to have to namespace the built in IPublishedContent properties so they don't clash. Then on the front end we can have these fields in a nested object something like:

{
  person {
	name,
	nodeData {
		name,
		id,
		sortOrder,
		url
	}
  }
}

or alternatively we can just prefix them all with "node" but I think this is less clear/nice:

{
	person {
		name,
		nodeName,
		nodeId,
		nodeSortOrder,
		nodeUrl
	}
}

[rasmusjp]

I thought about moving properties or the built in properties to a child field, but I actually think it would be better to either ensure you don't use the same names as the built in ones (which I believe the ModelsBuilder does) or simply ignore them and write it to the log.

[PeteDuncanson]

I know there are plans for Model Builder to do some thing similar to the above and move those fields into a Meta Data field (modelsbuilder/ModelsBuilder.Original#122 (comment)) so I'd be happy going with a nested object to hold these I think.

[rasmusjp]

We still need to consider that even if we move the properties to a nested field, someone could still create a property with that name and get an error (or a warning). The "safe" but annoying way is to move the custom properties to a nested field like properties, but i really don't like that as it will make the queries more verbose.

[PeteDuncanson]

We pondered internally having each field nested under its Tab but I'm not sure if that leaks too much knowledge about how the CMS is structured and the end user shouldn't need to know all that (plus what if you move a field to another tab)? Still don't know if this will fix the point you raise about still having clashes when trying to get the data with the same name. Seems to me that Core shouldn't allow you to name something the same as a core property, they could enforce this in the UI and Code of the back office.

I've been playing around with trying to get nodeData on a nested object for fun but its then trying to find a field on my content called "nodeData" which obviously doesn't exist. Not sure what I'm doing wrong. Might need you to give me some pointers or a walk though as to what I'm doing wrong.

[rasmusjp]

I don't like nesting each field under its Tab, mainly because I don't think its something the consumer needs to know about and we would also have to think about how to handle the tab names, since per the specs names can only contain alphanumeric characters and _,

Even if it was enforced in Core, we would still have problems with the custom fields like isFirst, isLast and isVisible, so moving them or the properties to a nested field might be the best solution unless we have a "big" list of reserved names (what would happend if I install the package on an existing site that uses a "reserved" name?).

I we do decide to move the built in properties to a nested field, we could choose a name that's unlikely to be used like _nodeData (not sure if i like that name since we aren't using nodes anymore, are we? 😉) and "reserve" that name.

I think to make the nested field work you need to implement the resolve delegate and just return context.Source like:
https://github.com/rasmusjp/umbraco-graphql/blob/25bc2583090b614d88dfb8d98e14f0db7bcb2a60/src/Our.Umbraco.GraphQL/Types/ComplexGraphTypeOfIPublishedContentExtensions.cs#L27
or
https://github.com/rasmusjp/umbraco-graphql/blob/25bc2583090b614d88dfb8d98e14f0db7bcb2a60/src/Our.Umbraco.GraphQL/Types/ComplexGraphTypeOfIPublishedContentExtensions.cs#L55
depending on how you add the field

[PeteDuncanson]

One "dirty" way around the nesting is to just add an underscore to all the reserved field names but I'm not sure I like it but the more I fight with getting what I thought would be a simple code hack together the more I'm coming around to it! I'm going to update and scrub what I've done and start again, learnt so much with my hacking around but need to keep it clean and have a fresh go at it. I'll see if I can't get nesting to work, might shout out for some more pointers if it causes me any more grief.

[PeteDuncanson]

Jack and I have been hacking around with this and got the NodeData as a nested field. Happy to change the name but its a proof of concept. Thoughts?

Offroadcode@f321849

Additionally it seems in recent versions you can't name your property types the same as the built in one which means we might get around the naming clash for new builds. Existing sites though might still have this issue. Trying to find the commit that fixes this in Umbraco source at the moment but no luck.

[rasmusjp]

I would probably rename the BuildInContentDataGraphTypeclass to PublishedContentDataGraphType and set the name to PublishedContentData other than that it looks great. We can always discuss the name, but I think this might be the way to go.

It looks like it's the Models Builder Validation that's preventing you from naming your property the same as a built in

[JackPenney]

You're right it was models builder that was preventing those aliases, testing with it disabled does confirm that this gets around the content clashing with the user created doctype aliases

Happy to give it what ever name suits it best, I've renamed it to your suggestion and created a pull request for the mod

#15

[rasmusjp]

Closing since #15 has been merged in and #42 has been raised instead.