Skip to content
This repository has been archived by the owner on Nov 23, 2021. It is now read-only.

Reworked archetype/template #629

Merged
merged 38 commits into from
May 17, 2018
Merged

Reworked archetype/template #629

merged 38 commits into from
May 17, 2018

Conversation

hohwille
Copy link
Member

@hohwille hohwille commented Jan 23, 2018

I am reworking our maven archetype (project template):

This PR is now complete. Still we can discuss to remove BinaryObject but IMHO it is easier to keep this out here and if we want to do that simply create another PR afterwards. Please still see my open issues in the list on the first comment.

@hohwille
Copy link
Member Author

hohwille commented Jan 26, 2018

To be honest: our archetype was/is kind of a mess (contains restaurant specific leftovers, code with smells and anti-patterns, etc.). I am still in the process of heavy rework. Contribution and feedback is most welcome.

  • we added security.basic.enabled=false that seems to imply that this property will trigger anything, however such feature was never implemented and therefore this only causing confusion. I will remove this crap.
  • we used http.mappers.jsonPrettyPrint=true this is deprecated and might not be supported anymore - see spring-projects/spring-boot@35b7ba5. Will fix this.
  • we created a class DatabaseMigrator that is entirely odd. It is not a spring bean and manually wires up a flyway instance from a given datasource. Will have to rewrite this as well.
  • we have strange leftovers from tries to make the DB configurable. This has to be done via configuration of the archetype but not as build or runtime configuration. A productive app will not support multiple databases in parallel by default. I will have to completely rework this.
  • we do have an option to create an enterpise/JEE project from the template. However, why do we need to configure the folder of the ear module? Isnt our entire approach about standardization? A big benefit of OASP is that you get standardized structures (packages, project layouts, etc.). So why do we actually give unnecessary flexibility by default here?
  • Futher, if we really want to support an enterprise/JEE project with an OASP4J template, what is the benefit of just adding an ear module but keeping all the rest as is. If you really want to build an EAR today - will you then use spring and spring-boot inside? IMHO this all makes absolutely no sense. Either we should drop this support at all or we should create a separate archetype for enterprise/JEE that is really designated to a reasonable JEE app server. I personally would better drop all this as we do not have real contributions from JEE guys here. If we have them they are more than welcome to contribute something that is really working but otherwise we are just providing something that might look like we have full support for it but under the hood there is pretty much nothing there.
  • we strongly push out batch stuff here with very little value (I am not talking about our guide and example but about our sole archetype). Many projects wont deal with batches and only get polluted with dependencies here. Either we drop this entirely and explain in the guide how to extend for batch or we should add an option via archetype variable that allows to enable/disable the batch stuff. In any way I would modularize the batch to a separate module.
  • I remove all the restaurant specific DDLs, masterdata, testdata, etc. Currently I left the BLOB table and its single masterdata entry in place. However, maybe even this is wrong to provide in an archetype as you should get an empty application rather than an example app (for the latter you can clone MTS).
  • we have dispatcher-servlet.xml that seems to be a leftover from the spring XML config but now is not used anymore (not referenced anywhere). So can we simply remove that? Yes, was not needed. I removed it meanwhile.
  • we have an index.html as well as an index.jsp in the app root. IMHO this does not make any sense. How to proceed? I removed index.jsp and login.jsp as it is not used anymore and was an obsolete leftover.
  • we do not follow our own packaging conventions. I will refactor this.
  • Why should we need a custom context.xml - IMHO this is even dangerous for IT security and should not come by default. I would rather delete the entire webapp folder.
  • so much more, see the diffs for details...

This was referenced Jan 26, 2018
@MarcoRose
Copy link
Member

MarcoRose commented Mar 1, 2018

Please add the maven license plugin with some configuration-options.
A user may then manually execute:

mvn license:aggregate-download-licenses 

and

mvn license:aggregate-add-third-party

The plugin can also be activated to run during build

      <plugin>
        <groupId>org.codehaus.mojo</groupId>
        <artifactId>license-maven-plugin</artifactId>
        <version>1.14</version>
        
        <!-- configuration outside of execution-tags will be applied for all calls of license-maven-plugin -->
        <configuration>
          <outputDirectory>${project.build.directory}/generated-resources</outputDirectory>
          <sortArtifactByName>true</sortArtifactByName>
          <includeTransitiveDependencies>true</includeTransitiveDependencies>
          <!-- the "missing file" declares licenses for dependencies that could not be detected automatically -->
          <!-- find the "missing files" in all child-projects at the following location -->
          <!-- if the "missing files" are not yet existing in child-projects they will be created automatically-->
          <useMissingFile>true</useMissingFile>
          <failOnMissing>true</failOnMissing>
          <missingFile>src/license/THIRD-PARTY.properties</missingFile> 
          <overrideFile>src/license/override-THIRD-PARTY.properties</overrideFile>
          <!-- harmonize different ways of writing license names -->
          <licenseMerges>
            <licenseMerge>Apache Software License, Version 2.0|The Apache Software License, Version 2.0</licenseMerge>
            <licenseMerge>Apache Software License, Version 2.0|Apache 2.0</licenseMerge>
            <licenseMerge>Apache Software License, Version 2.0|Apache License, Version 2.0</licenseMerge>
          </licenseMerges>
          <encoding>utf-8</encoding>
        </configuration>

        <!-- 
          In case you want to create license information during regular builds, uncomment below executions, 
          then run mvn package. This will create the THIRD-PARTY.txt and will
          download license files into target folder and create a licenses.xml -->
        <!--
        <executions>
          <execution>
            <id>aggregate-add-third-party</id>
            <phase>generate-resources</phase>
            <goals>
              <goal>aggregate-add-third-party</goal>
            </goals>
          </execution>
          
          <execution>
            <id>aggregate-download-licenses</id>
            <phase>generate-resources</phase>
            <goals>
              <goal>aggregate-download-licenses</goal>
            </goals>
          </execution>          
        </executions> 
        -->
      </plugin>

Thank you.

The src/license/THIRD-PARTY.properties file contains additional license declarations for used dependencies that can not automatically get detected by maven license plugin. The content is of course project-specific. For mts-core the following content is correct:
mtsj/core/src/license/THIRD-PARTY.properties

# Generated by org.codehaus.mojo.license.AddThirdPartyMojo
#-------------------------------------------------------------------------------
# Already used licenses in project :
# - ASF 2.0
# - Apache 2
# - Apache 2.0
# - Apache License 2.0
# - Apache License, Version 2.0
# - Apache License, version 2.0
# - Apache Software Licenese
# - Apache Software Licenses
# - BSD
# - BSD License
# - BSD style
# - BSD-like
# - CDDL + GPLv2 with classpath exception
# - CDDL 1.1
# - CDDL+GPL License
# - CDDL/GPLv2+CE
# - CPL
# - Common Development and Distribution License (CDDL) v1.0
# - Eclipse Distribution License (EDL), Version 1.0
# - Eclipse Public License (EPL), Version 1.0
# - Eclipse Public License - v 1.0
# - Eclipse Public License 1.0
# - Eclipse Public License v1.0
# - GNU Lesser General Public License
# - GPL2 w/ CPE
# - Indiana University Extreme! Lab Software License, vesion 1.1.1
# - LGPL 2.1
# - MIT License
# - MPL 1.1
# - MPL 2.0 or EPL 1.0
# - New BSD License
# - Public Domain
# - The Apache Software License, Version 2.0
# - The BSD License
# - The JSON License
# - The MIT License
#-------------------------------------------------------------------------------
# Please fill the missing licenses for dependencies :
#
#
#Wed Feb 14 11:11:15 CET 2018

#https://github.com/dom4j/dom4j/blob/master/LICENSE
dom4j--dom4j--1.6.1=BSD 3-Clause

javax.servlet--jstl--1.2=CDDL

#https://github.com/codehaus/jettison/blob/master/src/main/resources/META-INF/LICENSE
org.codehaus.jettison--jettison--1.2=Apache License, Version 2.0

…roved docs, tidy up

#384: extract api module properly
#401: #629: updated maven plugins, integrated owasp dependency check
@hohwille
Copy link
Member Author

hohwille commented Mar 2, 2018

We have a problem here:

  • CXF seems not to work properly in our setup if we split API from Impl of REST-services. For some strange reason CXF is creating a ClassResourceInfo for the service that has no operation resource infos (CXF can not find the JAX-RS annotated methods from the implemented interface). The strange thing is that the same works in my customer project but I can not figure the difference.
  • The actual problem was never revealed and still is not because the according test extends SubsystemTest that is not executed by default. IMHO we have to change this default as we changed the strategy for SubsystemTests. I will do that but then the tests will fail.

I would love to get some support or hints on this. It seems that we have a strange but severe issue here.
I debugged everything in detail:

I found the source of the actual problem:

public CsrfToken getCsrfToken(@Context HttpServletRequest request, @Context HttpServletResponse response) {

So I forgot to remove the @Context annotations after extracting the API to the interface.

@hohwille
Copy link
Member Author

hohwille commented Mar 2, 2018

Build broke because spring boot plugin already requires java8. This is actually not so great. Should I upgrade travis.yml or do we need to get stuck with old plugins because of Java7 support?

This is the failing class:
https://github.com/spring-projects/spring-boot/blob/master/spring-boot-project/spring-boot-tools/spring-boot-maven-plugin/src/main/java/org/springframework/boot/maven/RepackageMojo.java

So this is because of spring-boot-maven-plugin. However, IMHO I did not update this one.

UPDATE: I tied the version of this plugin via pluginManagement and now travis is green. However the bug was not reproducible locally even with JDK1.7. Seems that maven on travis behaves different than locally - strange. However, this way I was not forced to upgrade travis.yml to Java8 (we have to do that soon anyhow but then we will loose the assurance that our code will still work with Java7 as it may [accidentally] use new JDK1.8 types).

@hohwille
Copy link
Member Author

hohwille commented Mar 2, 2018

@MarcoRose I have integrated license-maven-plugin. To be honest I can not see a real benefit of this plugin over the already integrated Project Dependency Management. What is the real added value of this? Maybe I am missing some features...
It does not seem to judge on used licenses even the normalizing of the licenses is not properly honored. I will check if I messed this by reading the docs (http://www.mojohaus.org/license-maven-plugin/aggregate-third-party-report-mojo.html#licenseMerges) and trying to simplify your given config.

@hohwille
Copy link
Member Author

hohwille commented Mar 2, 2018

Both

        <configuration>
          <licenseMerges>
            <licenseMerge>Apache Software License, Version 2.0|The Apache Software License, Version 2.0</licenseMerge>
            <licenseMerge>Apache Software License, Version 2.0|Apache 2.0</licenseMerge>
            <licenseMerge>Apache Software License, Version 2.0|Apache License, Version 2.0</licenseMerge>
          </licenseMerges>
        </configuration>

as well as

            <licenseMerges>
              <licenseMerge>Apache Software License, Version 2.0|The Apache Software License, Version 2.0|Apache 2.0|Apache License, Version 2.0</licenseMerge>
            </licenseMerges>

Does not have any effect on the report. No matter if I put this into <configuration> of <pluginManagement> or <reporting> for the license-maven-plugin.
In any case I get the original licenses listed without normalization. IMHO this plugin is not so much of a benefit but correct me if I am missing some features...

@hohwille hohwille self-assigned this Mar 2, 2018
@hohwille
Copy link
Member Author

I can instantiate and run the archetype with oracle but booting the spring boot app leads to the same error that @vapadwal was stating.
Oracle is simply too limited to insert the BLOB this way:
https://stackoverflow.com/questions/13945710/error-ora-01704-string-literal-too-long

The workarounds would maybe fix this for oracle but again break it for other DBs. The easiest fix for now would be to simply put only a small BLOB (picture) in our masterdata SQL. Or as an alternative and as questioned from the start remove the entire BLOB stuff including all its code from the archetype.
I guess a regular user expects to get an empty application rather than a little demo how to manage BLOBs via REST API. For the moment and to finalize this PR (it has really a very long and bloody history) I would prefer to simply replace the BLOB value with something small that works in oracle and then finally complete this PR first by merging it.
Then we are free for another PR or issue about removing the BLOB stuff from archetype once more.

@hohwille
Copy link
Member Author

I was able to replace the BLOB with something smaller that is now working with oracle.
However, next I get this error from oracle:

Caused by: org.hibernate.tool.schema.spi.SchemaManagementException: Schema-validation: missing column [timestamp] in table [RevInfo]
	at org.hibernate.tool.schema.internal.SchemaValidatorImpl.validateTable(SchemaValidatorImpl.java:85) ~[hibernate-core-5.0.12.Final.jar:5.0.12.Final]
	at org.hibernate.tool.schema.internal.SchemaValidatorImpl.doValidation(SchemaValidatorImpl.java:50) ~[hibernate-core-5.0.12.Final.jar:5.0.12.Final]
	at org.hibernate.tool.hbm2ddl.SchemaValidator.validate(SchemaValidator.java:91) ~[hibernate-core-5.0.12.Final.jar:5.0.12.Final]
	at org.hibernate.internal.SessionFactoryImpl.<init>(SessionFactoryImpl.java:475) ~[hibernate-core-5.0.12.Final.jar:5.0.12.Final]
	at org.hibernate.boot.internal.SessionFactoryBuilderImpl.build(SessionFactoryBuilderImpl.java:444) ~[hibernate-core-5.0.12.Final.jar:5.0.12.Final]
	at org.hibernate.jpa.boot.internal.EntityManagerFactoryBuilderImpl.build(EntityManagerFactoryBuilderImpl.java:879) ~[hibernate-entitymanager-5.0.12.Final.jar:5.0.12.Final]
	... 22 common frames omitted

Actually the column timestamp is present in RevInfo but there is again some reserved keyword issue that we are facing here. Never ending story though...

@hohwille
Copy link
Member Author

Okay. Works with oracle if we put the column name in quotes: @column(""timestamp")
I hope this does not break the mapping for other DB types.

@hohwille
Copy link
Member Author

So now I am completed. I could not quickly test that it is still working with MS SQL-Server or postgres but it works with h2, hsqldb and oracle (tested with XE) so I guess it should also work for the others if it used to work before.

@hohwille
Copy link
Member Author

Build and tests worked locally with maven. No idea why it fails with travis now. This PR is really driving me nuts.

…ITALIZED for some strange reason and h2 treats names case-sensitive)
@vapadwal
Copy link
Contributor

vapadwal commented Apr 25, 2018

I have test with the new changes again and below are some more issues found

Issue 1:

Getting below exception when executed mvn install against oasp4j

apableBeanFactory.java:1624)
... 90 common frames omitted
Caused by: org.hibernate.tool.schema.spi.SchemaManagementException: Schema-validation: missing column [timestamp] in table [RevInfo]
at org.hibernate.tool.schema.internal.SchemaValidatorImpl.validateTable(SchemaValidatorImpl.java:85)
at
... 96 common frames omitted
[INFO] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 5.498 s - in io.oasp.gastronomy.restaurant.tablemanagement.service.impl.rest.TablemanagementRestServiceTest
[INFO]
[INFO] Results:
[INFO]
[ERROR] Errors:
[ERROR] SecurityRestServiceImplTest.testGetCsrfToken » IllegalState Failed to load App...
[ERROR] SecurityRestServiceImplTest.testGetCurrentUser » IllegalState Failed to load A...
[ERROR] SecurityRestServiceImplTest.testLogin » IllegalState Failed to load Applicatio...
[ERROR] TablemanagementWebServiceTest.testWebService » IllegalState Failed to load App...
[INFO]
[ERROR] Tests run: 43, Failures: 0, Errors: 4, Skipped: 1
[INFO]

I think we need timestamp in quotes. refer to below file

https://github.com/hohwille/oasp4j/blob/feature-template/samples/core/src/test/resources/db/tablemanagement/V0001__R001_Create_schema.sql#L134

Issue 2:

Created a project in h2 and oracle got the below error both in h2 and oracle

Caused by: org.hibernate.DuplicateMappingException: Table [RevInfo] contains logical column name [timestamp] referring to multiple physical column names: ["timestamp"], [timestamp]
at org.hibernate.boot.internal.InFlightMetadataCollectorImpl$TableColumnNameBinding.bindLogicalToPhysical(InFlightMetadataCollectorImpl.java:902) ~[hibernate-core-5.0.12.Final.jar:5.0.12.Final]

Because of the above issue I am not able to test further.

Issue 3 :

The project get created via console(using mvn -DarchetypeVersion=3.0.0-SNAPSHOT -...... command) but when tried it creating via eclipse got the below error

Unable to create project from archetype [io.oasp.java.templates:oasp4j-template-server:3.0.0-SNAPSHOT]
org.apache.maven.archetype.exception.ArchetypeGenerationFailure: Error merging velocity templates: Lexical error: org.apache.velocity.runtime.parser.TokenMgrError: Lexical error at line 31, column 4. Encountered: after : ""

Might be because of velocity error below

https://github.com/hohwille/oasp4j/blob/feature-template/templates/server/src/main/resources/archetype-resources/__batch__/src/test/java/__packageInPathFormat__/general/common/base/test/TestUtil.java#L31
or
https://github.com/hohwille/oasp4j/blob/feature-template/templates/server/src/main/resources/archetype-resources/core/src/test/java/__packageInPathFormat__/general/common/base/test/TestUtil.java#L31

…ITALIZED for some strange reason and h2 treats names case-sensitive)
@hohwille
Copy link
Member Author

hohwille commented Apr 27, 2018

@vapadwal thanks for testing and your feedback.
I fixed Issue 1. I already fixed the same issue in the template but did not hit it before with the sample. Further as you can verify the travis build of my change was also green so something is rather strange or evil here.

For issue 2 I have the opposite effect. It did not work without the quotes and therefore I did all the changes. I can test everything locally with OracleXE and it is working in my case. So maybe it works with my OralceXE version (11.2.0) but not with yours. In that case I rather have to give up for the moment. To repeat myself I just tried to pick up what @kiran-vadla once did here and it also worked perfectly for my local OracleXE. I am not sure how we should handle such cases in general but our problem somehow is that we have a buggy and crappy template currently but creating a PR to improve requires ultimate perfection and takes months or years. I guess most other contributors would have given up with their PR way before. So I guess I should also take the chance to open a community discussion outside this PR about how we want to work in such cases in the future. Maybe it is better to finally accept and merge PRs and rather open a follow up issue for further improvements than getting stuck so badly. Do not get me wrong here: Your feedback is valuable, perfect and helps to improve quality. Also I really messed up some things that you digged out and it is great that we fixed this before projects ran into the issue.

For issue 3 I am also a little bit puzzled but IMHO this is as following:
I upgraded the archetype plugin so we now have a newer velocity engine under the hood for our archetype that provides features to better deal with nasty escaping, etc. However, when instantiating the archetype under some circumstances maven uses an older version of the archetype plugin with an older version of velocity that then can not parse our templates resulting in that error.
This is good to know, but next we would need to understand under which circumstances this will happen:

  • Is it due to an older version of maven (can you please provide the output of mvn -v and check a recent maven version).
  • Does it depend on the versions of maven archetype plugin available in your local repo?

So to make things clear: This error does not happen when I test it. My environment:

mvn -v
Apache Maven 3.5.2 (138edd61fd100ec658bfa2d307c43b76940a5d7d; 2017-10-18T09:58:13+02:00)
Maven home: D:\Projekte\oasp\software\maven\bin\..
Java version: 1.8.0_161, vendor: Oracle Corporation
Java home: D:\Projekte\oasp\software\java\jre
Default locale: en_US, platform encoding: Cp1252
OS name: "windows 7", version: "6.1", arch: "amd64", family: "windows"

My invocation to create the project was e.g.:

mvn archetype:generate -DarchetypeGroupId=io.oasp.java.templates -DarchetypeArtifactId=oasp4j-template-server -DarchetypeVersion=3.0.0-SNAPSHOT -DgroupId=com.company -DartifactId=demo -Dversion=1.0.0-SNAPSHOT -DdbType=oracle -Dbatch=batch

If we can not proceed here we can as alternative drop all these features and simply remove the javadoc links entirely reducing the quality a little but getting away from this velocity issues.

@hohwille
Copy link
Member Author

IMHO the remaining question is if that trick/workaround/hack is actually evil:
537740f

If yes - what is the alternative to make this work with all the different databases. SQL aims to be a standard but actually it is not and is rather a pain.

@vapadwal
Copy link
Contributor

vapadwal commented May 3, 2018

For issue 3, i.e. velocity issue:
I am also able to create project successfully via mvn command line, but the issue is happening with me when trying to create project via eclipse wizard(File>New>Maven Project)

For timestamp Issue " what is the alternative to make this work with all the different databases "
If its possible, I think its better to rename the timestamp column just the way we did it for "size".

@hohwille
Copy link
Member Author

hohwille commented May 4, 2018

For timestamp Issue " what is the alternative to make this work with all the different databases "
If its possible, I think its better to rename the timestamp column just the way we did it for "size".

I agree that we should not use reserved keywords. However, this big issue here is that projects are already actively using this code and would then be forced to update their DBs in production. I do not see this as a good option right now - IMHO we would make some of our users mad and they might go away from Devon/OASP then... WDYT?

@vapadwal
Copy link
Contributor

vapadwal commented May 7, 2018

I have uninstall my oraclexe and again reinstalled it , Now its running properly in oracle (Not sure what was the problem there)

There is still issue with timestamp column running for H2. This is because of the changes (@column(name = ""timestamp"")) in AdvancedRevisionEntity.java
https://github.com/hohwille/oasp4j/blob/feature-template/modules/jpa-envers/src/main/java/io/oasp/module/jpa/dataaccess/api/AdvancedRevisionEntity.java#L43

As we are escaping timestamp we need to put quotes for timestamp in below file. This is for H2 case

https://github.com/hohwille/oasp4j/blob/feature-template/templates/server/src/main/resources/archetype-resources/core/src/main/resources/db/type/h2/V0002__Create_RevInfo.sql#L4

Now with this changes all the timestamp issue will be fixed.

@hohwille hohwille modified the milestones: oasp:EVE, oasp:2.6.0 May 8, 2018
@hohwille hohwille changed the base branch from develop to develop-2.x May 8, 2018 08:02
hohwille added 2 commits May 8, 2018 10:06
…s with older versions of velocity still present in e.g. m2e preventing the archetype project to be properly created.
@hohwille
Copy link
Member Author

hohwille commented May 8, 2018

@vapadwal thanks for your patience and continuous testing and reviewing this PR. It might have been tedious but I hope we now finally got it well!

I am also able to create project successfully via mvn command line, but the issue is happening with me when trying to create project via eclipse wizard(File>New>Maven Project)

I fixed this be stop using this advanced velocity feature and simply kick out the javadoc link that caused velocity to fail.

Now with this changes all the timestamp issue will be fixed.

I also fixed this last occurrence. I missed it because it is not used by the tests but "only" if you actually run the archetype project with h2 what I never properly tested in the hurry. Thanks for your excellent quality assurance!

@hohwille
Copy link
Member Author

hohwille commented May 8, 2018

Fine now we have a technical travis issue:

Caused by: org.eclipse.aether.transfer.ArtifactNotFoundException: Failure to find io.oasp.java:oasp4j-bom:pom:3.0.0-SNAPSHOT in https://oss.sonatype.org/content/repositories/snapshots/ was cached in the local repository, resolution will not be reattempted until the update interval of sonatype-snapshots has elapsed or updates are forced

@AbhayChandel
Copy link
Contributor

Tested the PR and works fine.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants