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

Clarify nullability of ProjectDescription attributes #974

Open
snicoll opened this issue Aug 6, 2019 · 6 comments
Open

Clarify nullability of ProjectDescription attributes #974

snicoll opened this issue Aug 6, 2019 · 6 comments

Comments

@snicoll
Copy link
Contributor

snicoll commented Aug 6, 2019

Right now, platformVersion is expected to be non null in various contributors but there is nothing that really enforces that.

We should review the various attributes, document them and enforce non nullability when the description is resolved.

@snicoll
Copy link
Contributor Author

snicoll commented Aug 6, 2019

While this could be mitigated with the fact that contributors may infer a default, ResolvedProjectDescription is an immutable model of the project request and, as such, should define those aspects consistently. Whatever contributors are at play should run before the description is resolved.

@snicoll
Copy link
Contributor Author

snicoll commented Aug 7, 2019

On the other hand, it's hard to force an attribute of the project to be set if the contributors at play don't need it. We can imagine a custom instance using a fixed platformVersion or a custom instance that does not generate "Applications" and therefore don't need an applicationName.

Flagging for team attention to see what the rest of the team thinks.

@snicoll
Copy link
Contributor Author

snicoll commented Aug 7, 2019

I am back with the idea of a ProjectDescriptionResolver that would resolve and apply sensible defaults if necessary. A default that doesn't know about the metadata can be provided and share a number of features that are currently hard-coded in the metadata such as cleaning package names and checking that applicationName is a valid identifier.

Doing so would provide sensible defaults for all properties if none are provided and let users change that to whatever custom implementation they want if necessary.

@snicoll
Copy link
Contributor Author

snicoll commented Aug 12, 2019

Some more brainstorming. I am now considering a ProjectDescriptionFiller that would be called at the end of the customization lifecycle and would "fill" the ProjectDescription with whatever isn't set yet (set defaults). If no such bean is present in the context, ProjectGenerator would contribute one that sets some basic defaults.

We can't really make the ResolvedProjectDescription attributes mandatory otherwise. Some attributes may not be used depending on the available contributors. For a manual use of the API, you could create an empty project with no information whatsoever.

The filler implementation can be swapped to provide useful defaults (potentially with some more complex logic similar to what ProjectRequestToProjectDescriptionConverter does at the moment. And it is always possible to chain filler, applying custom logic first and then fallback defaults if certain attributes aren't set.

The alternative to this is to check if the value is set and then fail or back-off. Backing off sounds interesting to some use cases (like not generating a build file if the build system isn't set) but that would probably require a way to notice what happened (what the auto-configuration report does at the moment in Spring Boot).

@snicoll
Copy link
Contributor Author

snicoll commented Aug 19, 2019

We discussed this on the call today and it looks like a process in two steps could be more interesting. One step to fill the ProjectRequest with defaults if necessary and one step to validate it. Each step could be replaced and a default is provided. This would let each infrastructure to decide what is mandatory, what isn't and which properties can have an acceptable default if no value is provided.

There are two things that are bugging me with this approach:

  • The nullability of an attribute is dependant on the infrastructure (what the validator implementation does) with no obvious way to get that information from the code
  • If a validator decides a particular item can be null, it takes this decision knowing this is acceptable. To take that decision, I feel it has to know which contributors are available. This doesn't sound like a scalable model to me.

What I had in mind initially was to decide:

  • Which attributes are optional or, rather can be reasonabily infered from other attributes if not set and set that in stone. Concretely ResolvedProjectDescription is updated so that each field indicates if it can be null or not. Null checks are performed when building the object.
  • What defaults should be applied for non-optional attributes if they aren't set

The downside of this approach is that the description is potentially "filled" with information contributors don't need. It is annoying but having a unique nullability description of each attributes feels more important to me.

Taking a concrete example. Let's assume we decide language should be mandatory. It feels weird to have to set one language for every infrastructure (simpler instance may not write code at all and just have a build file). A default implementation can "fill" this attribute if not set, defaulting to Java for instance. If you want to make sure language is always set for you particular instance, then a default implementation can be provided that doesn't set it.

Perhaps this issue is complicated because the concept of ResolvedProjectDescription where all features of a project are listed isn't very scalable either.

@mbhave
Copy link
Contributor

mbhave commented Aug 20, 2019

Thanks for the summary @snicoll. I'm actually leaning towards the first approach that we discussed this morning. I think rather than setting if the attribute is optional or not in stone, it seems better to let the infrastructure decide what's mandatory and what's not. To your point about the nullability then being dependent on the infrastructure, I think that's acceptable because the infrastructure defines what assets should be contributed to the project. It addresses the issue for simpler infrastructures, the ones that contribute only a build file for example, and they don't need to provide defaults or validate other fields.

@snicoll snicoll changed the title Clarify nullability of ResolvedProjectDescription attributes Clarify nullability of ProjectDescription attributes Aug 25, 2019
sayeedap pushed a commit to sayeedap/initializr that referenced this issue Sep 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants