[WIP] core: Ensure that DataSourcesMap is handled during apply w/destroy

[vancluever]

Fixes #6881, which came up for me in #6911 as well.

WIP because I'm still trying to find a good way to test the *schema.Provider part.

[apparentlymart]

@vancluever do you think this is the same issue @jen20 was trying to address in #6907?

We noticed during some debugging that we're generating "destroy" and "destroy tainted" nodes for data resources, which isn't supposed to happen and isn't handled properly in core... the "destroy" nodes always do the managed resource behavior, regardless of resource type.

He ultimately decided that this wasn't the right fix and we didn't discuss why, but just want to make sure we're not trying to solve the same problem in a multitude of different ways...

[vancluever]

@apparentlymart this is a little different - the intention isn't to do anything with the graph, but one thing I did notice is that *schema.Provider.Apply() definitely does not look at the DataSourcesMap when it does its thing, and I didn't necessarily see anything that would lead me to believe that it would only apply to destroys, so it might have been something that should be checked anyway.

If anything I'd say the latter part is more around fixing the destroy walk issue, where we just ignore a missing Delete function and just proceed as normal. I'd image that won't be needed if a destroy walk is not happening on data sources.

I did notice that @jen20 did say that the destroy stuff was needed in some respect, I figured that you guys might have talked about it though :)

Some additional thoughts: I'm not too sure if ignoring Destroy when deleting is the best idea, as it might create a case where bugs with resources (versus data sources) that are not properly designed don't panic when they should, and I'm wondering if the logic put into *schema.Provider.Apply() should be put into the other functions there as well (namely *schema.Provider.Diff() and *schema.Provider.Refresh()).

[apparentlymart]

The intention here is that we never enter schema.Provider.Apply for a data resource, since they are supposed to use the separate function ReadDataApply. So if we are ending up in this codepath for data resources then that's a bug in itself , and I think what @jen20 was looking at for #6907.

If I'm understanding this patch correctly then it makes it illegal to have both a resource and a data source both called aws_ami, which is in conflict with the intent here: we actually want to have matching names, so that e.g. resource "aws_ami" will create and manage an AMI while data "aws_ami" will retrieve the attributes of one that already exists, ideally with the attributes as consistent as possible between the two.

I suspect that whatever patch replaces #6907 will make this patch moot, since it will prevent us from reaching any of the branches you've created here... but we can wait until @jen20 has found a new path and confirm if this remains true.

[vancluever]

Yeah, this is something I was wondering about. (I was thinking about renaming the data source in #6911 to aws_ami, incidentally).

Maybe my efforts are better focused on trying to figure out why the root of the issue exists then versus trying to work around it... seeing as I am still trying to figure out how to test schema.Provider.Apply properly and coming up blank, I'm not going to complain!

[vancluever]

I think I've found the root cause, closing this now and will be putting in another PR in a sec.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.