-
-
Notifications
You must be signed in to change notification settings - Fork 969
7.1.x cli add-field for domain field additions #15289
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
base: 7.1.x
Are you sure you want to change the base?
Conversation
… the command line
|
|
||
| implementation project(':grails-forge-core') | ||
| implementation "org.apache.grails.bootstrap:grails-bootstrap:$projectVersion" | ||
| implementation "org.codehaus.groovy:groovy:$groovyVersion" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Groovy 4.x is org.apache.groovy! This is polluting the classpath.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is in the forge-cli, which is still a micronaut app and on groovy 3.
jdaugherty
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only concern I have with this PR is the decision to put it in grails-bootstrap. I am assuming end user apps won't be generating fields at runtime of a Grails application so this is the wrong spot to place it.
| * @since 7.0 | ||
| */ | ||
| @CompileStatic | ||
| class DomainFieldModifier { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you help me understand why this is in bootstrap and not the CLI itself? Current files in this project all relate to classes that need shared across grails-core, gradle, and gorm. I only see you using this in the CLI libraries, why not put it in one of those or add a new project that's shared between the CLI for common dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the grails.codegen.model package is in grails-bootstrap. This is adding those 2 generation classes to this package that already exists. This needs to be shared across BOTH clis and profiles.
grails-bootstrap is already a dependency of grails-shell-cli, putting the classes there means:
- profiles can use it (via shell-cli → bootstrap)
- shell-cli can use it (already depends on bootstrap)
- forge-cli can use it (just needs to depend on bootstrap which is part of this PR)
|
|
||
| domainDir.eachFileRecurse { File file -> | ||
| if (file.name == "${simpleClassName}.groovy") { | ||
| found = file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if there are duplicate class names across different packages? I think you had an example where you had an Admin & User view - couldn't multiple domains exist in that case with a different table name configuration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added duplicate domain name / different package support
| } | ||
|
|
||
| for (FieldNode field : classNode.fields) { | ||
| if (field.name == fieldName && !field.name.startsWith('$') && !field.name.startsWith('__')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know Grails uses these prefix conventions for fields that should not be found, but can we at least extract these prefixes into a variable? Can you check if such a variable already exists so we don't forget to change this file over time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
|
|
||
| return false | ||
| } catch (Exception e) { | ||
| return domainFile.text.contains("${fieldName}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it refuses to parse, shouldn't we error? What's the scenario here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
| } else { | ||
| int constraintBlockIndex = fieldInsertIndex + 1 | ||
| lines.add(constraintBlockIndex, '') | ||
| lines.add(constraintBlockIndex + 1, ' static constraints = {') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't formatting be handled independently of line addition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you suggesting to reformat the entire file after making changes?
| * Parses the Groovy source file and returns the main class node. | ||
| */ | ||
| private ClassNode parseClass(File sourceFile) { | ||
| CompilerConfiguration config = new CompilerConfiguration() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the context of where this is supposed to be used? We usually used forked threads so not to continue expand the memory of the running process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parseClass() is only called by fieldExists() which parses a single domain class file per add-field invocation
| BOTH | ||
| } | ||
|
|
||
| static final Set<String> SUPPORTED_TYPES = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only the built in types? Seems like enum would be a reasonable one ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was intentionally trying to keep this simple for the first phase. I've expanded it a bit.
|
|
||
| implementation project(':grails-forge-core') | ||
| implementation "org.apache.grails.bootstrap:grails-bootstrap:$projectVersion" | ||
| implementation "org.codehaus.groovy:groovy:$groovyVersion" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is in the forge-cli, which is still a micronaut app and on groovy 3.
add-field CLI Command
Overview
The add-field command adds a new field to an existing Grails domain class, optionally with validation
constraints using either Grails constraints or Jakarta Validation annotations.
Usage
grails add-field FIELD:TYPE [OPTIONS]
Arguments
Supported Field Types
Options
@NotNullor nullable: false)@NotBlankor blank: false)Constraint Styles
@NotNull,@NotBlank,@Size)Examples
Basic field (no constraints)
add-field Book title:String
Result:
With Grails constraints (default)
add-field Book title:String --not-nullable --max-size 255
Result:
With Jakarta Validation annotations
add-field Book title:String --not-nullable --not-blank --max-size 255 --constraint-style jakarta
Result:
With both constraint styles
add-field Book title:String --not-nullable --max-size 255 --constraint-style both
Result:
Constraint Mapping
Error Handling