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

ResourcePatternHints should not be merged in a single json entry #29270

Closed
snicoll opened this issue Oct 6, 2022 · 4 comments
Closed

ResourcePatternHints should not be merged in a single json entry #29270

snicoll opened this issue Oct 6, 2022 · 4 comments
Labels
status: invalid An issue that we don't feel is valid theme: aot An issue related to Ahead-of-time processing type: bug A general bug

Comments

@snicoll
Copy link
Member

snicoll commented Oct 6, 2022

As reported by #28977

The ResourcePatternHints is a container for includes and excludes and multiple ResourcePatternHints can be registered. It's possible that callers may expect that these includes and excludes are related, however, when the JSON is written all excludes and all includes are included together. See ResourceHintsPredicates.AggregatedResourcePatternHints for an example of the flattening that needs to occur when using the pattern hints.

@snicoll snicoll added type: bug A general bug theme: aot An issue related to Ahead-of-time processing labels Oct 6, 2022
@snicoll snicoll added this to the 6.0.x milestone Oct 6, 2022
@sdeleuze sdeleuze modified the milestones: 6.0.x, 6.1.x Feb 21, 2023
@bdshadow
Copy link
Contributor

I want to help with this, but please advise on what is expected.
For example, let's say that we have the following resource hints:

ResourceHints hints = new ResourceHints();
hints.registerPattern(hint -> hint.includes("com/*").excludes("com/example/*"));
hints.registerPattern(hint -> hint.includes("com/example/*/all.properties").excludes("com/example/ignore/all.properties"));

Currently, it produces:

{
	"resources": {
		"includes": [
			{ "pattern": "\\Q/\\E"},
			{ "pattern": "\\Qcom/\\E.*"},
			{ "pattern": "\\Qcom\\E"},
			{ "pattern": "\\Qcom/example/\\E.*\\Q/all.properties\\E"},
			{ "pattern": "\\Qcom/example\\E"}
		],
		"excludes": [
			{ "pattern": "\\Qcom/example/\\E.*"},
			{ "pattern": "\\Qcom/example/ignore/all.properties\\E"}
		]
	}
}

Am I right that it's supposed to be the following way:

{
	"resources": {
		"includes": [
			{ "pattern": "\\Q/\\E"},
			{ "pattern": "\\Qcom/\\E.*"},
			{ "pattern": "\\Qcom\\E"},
		],
		"excludes": [
			{ "pattern": "\\Qcom/example/\\E.*"},
			{ "pattern": "\\Qcom/example/ignore/all.properties\\E"}
		]
	}
}

I think it can be quite complicated in some cases to filter it out properly. However, a simple filter is possible - for example, if it's the same or if a particular "include" is of a particular pattern of some "exclude" - then remove this "include".

Alsoб just for a note, here is the corresponding implementation in the graalvm: https://github.com/oracle/graal/pull/2912/files#diff-4b973ff5fd08418824ad1f4c7ce54a757b0641094f899f02a1c54fdc112739f2R263. It goes the same way as ResourceHintsPredicates.AggregatedResourcePatternHints, so making such filtering is better but not necessary. I meanб this ticket doesn't look like a bug to me (but probably, I'm missing smth).

@snicoll
Copy link
Member Author

snicoll commented Oct 4, 2023

@bdshadow you are correct. I think the assumption made in #28977 was wrong. I think @philwebb thought that you could register an array of includes/excludes pair but, looking at the documentation it really is an array of includes and an array of excludes.

As such, I believe the current behavior is correct based on that constraints. Thanks for bringing this up!

@snicoll snicoll closed this as not planned Won't fix, can't repro, duplicate, stale Oct 4, 2023
@snicoll snicoll removed this from the 6.1.x milestone Oct 4, 2023
@snicoll snicoll added the status: invalid An issue that we don't feel is valid label Oct 4, 2023
@philwebb
Copy link
Member

philwebb commented Oct 4, 2023

I think @philwebb thought that you could register an array of includes/excludes pair

No, I didn't think that, but I was worried that users might as include and exclude are on ResourcePatternHints. I think I was thinking a ResourceHints.registerInclude and ResourceHints.registerExclude method might be better to show that includes/excludes are completely independent.

Given that the API is out there and nobody seems to have hit the issue it looks like my fears were unfounded.

@bclozel
Copy link
Member

bclozel commented Oct 4, 2023

Excludes will go away in #31340 as GraalVM will simplify support on the resource-config format.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: invalid An issue that we don't feel is valid theme: aot An issue related to Ahead-of-time processing type: bug A general bug
Projects
None yet
Development

No branches or pull requests

5 participants