Mcpods 6535 validate rest against psql #112

Amaneusz · 2023-11-30T14:14:44Z

No description provided.

github-actions · 2023-11-30T19:58:41Z

Code Coverage

Overall Project	31.59% `-0.07%`	🍏
Files changed	29.03%	🍏

Module	Coverage
here-naksha-app-service	51.73% `-0.09%`	🍏
here-naksha-lib-hub	35.19% `-1.28%`	🍏

Files

Module	File	Coverage
here-naksha-app-service	NakshaApp.java	67.24% `-1.04%`	🍏
here-naksha-lib-hub	NHSpaceStorage.java	76.51% `-4.03%`	🍏
	NakshaHubConfig.java	57.78%	🍏
	NHSpaceStorageReader.java	54.57%	🍏
	NakshaHub.java	0.82% `-5.57%`	🍏
	ConfigUtil.java	0%	🍏
	NHAdminStorage.java	0% `-10.64%`	🍏

github-actions · 2023-11-30T20:02:10Z

Code Coverage

Overall Project	31.57% `-0.07%`	🍏
Files changed	13.73%	🍏

Module	Coverage
here-naksha-app-service	51.73% `-0.09%`	🍏
here-naksha-lib-hub	34.95% `-1.29%`	🍏

Files

Module	File	Coverage
here-naksha-app-service	NakshaApp.java	67.24% `-1.04%`	🍏
here-naksha-lib-hub	NHSpaceStorage.java	76.51% `-4.03%`	🍏
	NakshaHubConfig.java	57.78%	🍏
	NakshaHub.java	0.82% `-5.57%`	🍏
	ConfigUtil.java	0%	🍏
	NHAdminStorage.java	0% `-10.64%`	🍏

hirenkp2000

I would like to review the changes again, once review comments are addressed.

hirenkp2000 · 2023-12-01T05:31:10Z

here-naksha-app-service/src/test/java/com/here/naksha/app/service/UpdateFeatureTestHelper.java

Is it just the indention changes, in this file?

yes, some autoformat got applied accidentally - I am reverting this to make review less confusing

hirenkp2000 · 2023-12-01T05:41:07Z

here-naksha-lib-hub/src/main/java/com/here/naksha/lib/hub/NakshaHub.java

@@ -87,7 +92,8 @@ public NakshaHub(
      final @Nullable NakshaHubConfig customCfg,
      final @Nullable String configId) {
    // create storage instance upfront
-    this.psqlStorage = new PsqlStorage("naksha-admin-db", appName, "TODO", config);
+    this.psqlStorage =
+        new PsqlStorage("naksha-admin-db", appName, "local_test_" + System.currentTimeMillis(), config);


(this was a comment on last PR as well)
We need to support custom schema name supplied to the application as part of DB URL. Current logic of using random schema, won't work in cloud. We preferably fix it as part of this PR itself, OR pick it up immediately after this PR. But, let's not delay it for long, as from this point onwards, we will always be using PostgreSQL.

Once this resolved, we need to remove hubClassName property from cloud-config file (deployment/codedeploy/contents/naksha-hub/.config/cloud-config.json), so that Naksha service in cloud can also start working against actual postgres implementation, using consistent schema.

I forgot about "local_test_" + System.currentTimeMillis() - I'll get on it

hirenkp2000 · 2023-12-01T06:16:27Z

here-naksha-app-service/src/main/resources/config/mock-config.json

While I would have also preferred config files into specific sub-directories, it is not straight-forward to work at the moment. For example, try bringing your app on local from CLI and it will break:
java -jar build/libs/naksha-2.0.8-all.jar test-config

If you provide config directory, like this java -jar build/libs/naksha-2.0.8-all.jar config/test-config then it will work, but then it is not an ideal Config ID (due to presence of folder name) and also it will not seamlessly work for custom NAKSHA_CONFIG_PATH environment path and user home directory, due to additional folder name that it requires now.

It is surely possible to fix, but will require more refactoring to support loading config from all the possible places which we support right now and still respecting additional config folder. Right now, couple of options already has stopped working (specifically with custom env path and default user home directory).

Hence, my suggestion is, let's park this for now (if you agree?) and continue with the config files at root resources level itself. We anyways will remove some of these configs very soon, once postgres is fully stable.

let's park this for now (if you agree?) and continue with the config files at root resources level itself

I'm fine with that if we agree to address it in the future. I will revert this

Yes, absolutely. Let's create a ticket, which will go into next quarter and at that point depending on how many config files we are left with, we can readdress it.

hirenkp2000 · 2023-12-01T06:50:42Z

here-naksha-app-service/src/test/java/com/here/naksha/app/service/UpdateFeatureTestHelper.java

We will also require few test cases to validate transactional atomicity of REST APIs. But this can be addressed separately. I have raised ticket for the same - https://devzone.it.here.com/jira/browse/MCPODS-6665

github-actions · 2023-12-01T13:20:38Z

Code Coverage

Overall Project	31.55% `-0.12%`	🍏
Files changed	47.45%	🍏

Module	Coverage
here-naksha-app-service	51.67% `-0.09%`	🍏
here-naksha-lib-hub	34.97% `-1.22%`	🍏
here-naksha-lib-psql	21.26% `-0.33%`	🍏

Files

Module	File	Coverage
here-naksha-app-service	AbstractApiTask.java	75.25%	🍏
	NakshaApp.java	66.78% `-1.05%`	🍏
here-naksha-lib-hub	NHSpaceStorage.java	76.51% `-4.03%`	🍏
	NakshaHubFactory.java	75.61%	🍏
	NakshaHubConfig.java	57.78%	🍏
	NakshaHubMock.java	26.36%	🍏
	NakshaHub.java	0.83% `-5.18%`	🍏
	ConfigUtil.java	0%	🍏
	NHAdminStorage.java	0% `-10.64%`	🍏
here-naksha-lib-psql	PsqlStorage.java	0% `-4.5%`	🍏

hirenkp2000

Let's use constant for naksha-admin-db. It is already defined in PsqlStorage.ADMIN_STORAGE_ID. Also, local execution is failing as it seems id and app are required as part of DB URL, so we need to specifically change NakshaApp.DEFAULT_URL to add default values for app and id.

Also all tests need to start using actual postgres based custom storage (instead of current mock storage). for example - unit_test_data/ReadFeatures/ByIds/TC0400_ExistingAndMissingIds/create_storage.json

…38/51

…kill on pg instance in test 401 & following

…ctor issue (PluginCache)

…on bug

…tup)

hirenkp2000 · 2023-12-08T14:51:05Z

here-naksha-app-service/src/main/java/com/here/naksha/app/service/NakshaApp.java

  private static final String DEFAULT_URL =
-      "jdbc:postgresql://localhost:5432/postgres?user=postgres&password=pswd&schema=naksha";
+      "jdbc:postgresql://localhost:5432/postgres?user=postgres&password=pswd&schema=" + DEFAULT_SCHEMA;


also include app and id in default URL, else java -jar with default URL won't work on local.

hirenkp2000 · 2023-12-08T15:04:16Z

here-naksha-lib-core/src/main/java/com/here/naksha/lib/core/models/PluginCache.java

@@ -175,19 +179,24 @@ static class StorageConstructorByClassNameMap extends ConcurrentHashMap<String,
   */
  static <CONFIG> @Nullable Fe1<IStorage, CONFIG> wrapStorageConstructor(
      @NotNull Constructor<? extends IStorage> constructor, @NotNull Class<CONFIG> configClass) {
+    log.info("Wrapping constructor: {}", constructor);


should we convert logs in the method as DEBUG, as tests are stable now? (in cloud we will use INFO logging)

hirenkp2000 · 2023-12-08T15:08:45Z

here-naksha-lib-hub/src/main/java/com/here/naksha/lib/hub/NakshaHub.java

-          throw unchecked(new Exception(
-              "Unable to read custom/default config from Admin DB - ResultCursor has no data"));
+          logger.info("Not found any db configuration in '" + NakshaAdminCollection.CONFIGS
+              + "' admin collection");


After this step, app must have one config to continue with startup. And if config is not available then App should fail. So, I think we should continue with exception, unless I am missing reason for ignoring it now.

hirenkp2000 · 2023-12-08T15:12:15Z

here-naksha-lib-psql/src/main/java/com/here/naksha/lib/psql/PostgresStorage.java

@@ -91,7 +91,10 @@ private static long value(@Nullable Long value, long minValue, long defaultValue
    this.logLevel = logLevel == null ? EPsqlLogLevel.OFF : logLevel;
    this.masterConfig = masterConfig;
    if (masterConfig != null) {
+      log.info("creating PostgresStorage with masterConfig, storageId: {}, config: {}", storageId, masterConfig);


avoid logging config object (as it can leak password)

github-actions · 2023-12-08T16:14:00Z

Code Coverage

Overall Project	32.52% `-0.47%`	🍏
Files changed	52.99%	🍏

Module	Coverage
here-naksha-app-service	52.67% `-0.27%`	🍏
here-naksha-lib-core	31.66% `-0.26%`	🍏
here-naksha-lib-hub	26.29% `-4.14%`	🍏
here-naksha-lib-psql	21.78% `-0.48%`	🍏

Files

Module	File	Coverage
here-naksha-app-service	UrlUtil.java	100%	🍏
	EventHandlerApiTask.java	94.09%	🍏
	StorageApiTask.java	94.09%	🍏
	SpaceApiTask.java	94.09%	🍏
	WriteFeatureApiTask.java	77.85%	🍏
	AbstractApiTask.java	75.25%	🍏
	ReadFeatureApiTask.java	73.45%	🍏
	NakshaApp.java	65.29% `-3.26%`	🍏
here-naksha-lib-core	Position.java	36.32%	🍏
	PluginCache.java	18.62% `-10.27%`	🍏
	StorageException.java	0%	🍏
	StorageLockTimeout.java	0%	🍏
	StorageNotInitialized.java	0%	🍏
	SpaceProperties.java	0% `-84.62%`	🍏
	EventHandlerProperties.java	0% `-47.83%`	🍏
here-naksha-lib-hub	NHSpaceStorage.java	74.51% `-7.84%`	🍏
	NHAdminWriterMock.java	32.2% `-1.07%`	🍏
	NHAdminReaderMock.java	3.25% `-2.44%`	🍏
	NakshaHubConfig.java	1.78% `-1.33%`	🍏
	NakshaHub.java	0.82% `-4.53%`	🍏
	NakshaHubFactory.java	0% `-52.44%`	🍏
	ConfigUtil.java	0%	🍏
	NHAdminStorage.java	0% `-38.46%`	🍏
	NakshaHubMock.java	0% `-5.68%`	🍏
here-naksha-lib-psql	PsqlCursor.java	88.82% `-2.63%`	🍏
	PsqlStorage.java	0% `-4.5%`	🍏
	EPsqlState.java	0% `-3.17%`	🍏

…"psql"

github-actions · 2023-12-08T17:28:56Z

Code Coverage

Overall Project	32.55% `-0.59%`	🍏
Files changed	47.95%	🍏

Module	Coverage
here-naksha-app-service	52.67% `-0.28%`	🍏
here-naksha-lib-core	31.66% `-0.26%`	🍏
here-naksha-lib-hub	26.73% `-6.59%`	🍏
here-naksha-lib-psql	21.78% `-0.48%`	🍏

Files

Module	File	Coverage
here-naksha-app-service	UrlUtil.java	100%	🍏
	EventHandlerApiTask.java	94.09%	🍏
	StorageApiTask.java	94.09%	🍏
	SpaceApiTask.java	94.09%	🍏
	WriteFeatureApiTask.java	77.85%	🍏
	AbstractApiTask.java	75.25%	🍏
	ReadFeatureApiTask.java	73.45%	🍏
	NakshaApp.java	65.25% `-3.41%`	🍏
here-naksha-lib-core	Position.java	36.32%	🍏
	PluginCache.java	18.62% `-10.27%`	🍏
	StorageException.java	0%	🍏
	StorageLockTimeout.java	0%	🍏
	StorageNotInitialized.java	0%	🍏
	SpaceProperties.java	0% `-84.62%`	🍏
	EventHandlerProperties.java	0% `-47.83%`	🍏
here-naksha-lib-hub	NHSpaceStorage.java	74.51% `-7.84%`	🍏
	NHAdminWriterMock.java	32.2% `-1.07%`	🍏
	NHAdminReaderMock.java	3.25% `-2.44%`	🍏
	NakshaHubConfig.java	1.78% `-1.33%`	🍏
	NakshaHub.java	0.92% `-20.82%`	🍏
	NakshaHubFactory.java	0% `-52.44%`	🍏
	ConfigUtil.java	0%	🍏
	NHAdminStorage.java	0% `-38.46%`	🍏
	NakshaHubMock.java	0% `-5.68%`	🍏
here-naksha-lib-psql	PsqlCursor.java	88.82% `-2.63%`	🍏
	PsqlStorage.java	0% `-4.5%`	🍏
	EPsqlState.java	0% `-3.17%`	🍏

github-actions · 2023-12-08T17:32:17Z

Code Coverage

Overall Project	32.54% `-0.59%`	🍏
Files changed	47.95%	🍏

Module	Coverage
here-naksha-app-service	52.67% `-0.28%`	🍏
here-naksha-lib-core	31.66% `-0.26%`	🍏
here-naksha-lib-hub	26.71% `-6.58%`	🍏
here-naksha-lib-psql	21.78% `-0.48%`	🍏

Files

Module	File	Coverage
here-naksha-app-service	UrlUtil.java	100%	🍏
	EventHandlerApiTask.java	94.09%	🍏
	StorageApiTask.java	94.09%	🍏
	SpaceApiTask.java	94.09%	🍏
	WriteFeatureApiTask.java	77.85%	🍏
	AbstractApiTask.java	75.25%	🍏
	ReadFeatureApiTask.java	73.45%	🍏
	NakshaApp.java	65.25% `-3.41%`	🍏
here-naksha-lib-core	Position.java	36.32%	🍏
	PluginCache.java	18.62% `-10.27%`	🍏
	StorageException.java	0%	🍏
	StorageLockTimeout.java	0%	🍏
	StorageNotInitialized.java	0%	🍏
	SpaceProperties.java	0% `-84.62%`	🍏
	EventHandlerProperties.java	0% `-47.83%`	🍏
here-naksha-lib-hub	NHSpaceStorage.java	74.51% `-7.84%`	🍏
	NHAdminWriterMock.java	32.2% `-1.07%`	🍏
	NHAdminReaderMock.java	3.25% `-2.44%`	🍏
	NakshaHubConfig.java	1.78% `-1.33%`	🍏
	NakshaHub.java	0.91% `-20.68%`	🍏
	NakshaHubFactory.java	0% `-52.44%`	🍏
	ConfigUtil.java	0%	🍏
	NHAdminStorage.java	0% `-38.46%`	🍏
	NakshaHubMock.java	0% `-5.68%`	🍏
here-naksha-lib-psql	PsqlCursor.java	88.82% `-2.63%`	🍏
	PsqlStorage.java	0% `-4.5%`	🍏
	EPsqlState.java	0% `-3.17%`	🍏

Amaneusz force-pushed the MCPODS-6535-validate_rest_against_psql branch from eb5560b to 3782cd1 Compare November 30, 2023 20:00

Amaneusz marked this pull request as ready for review November 30, 2023 20:02

hirenkp2000 requested changes Dec 1, 2023

View reviewed changes

Amaneusz force-pushed the MCPODS-6535-validate_rest_against_psql branch from 43fe86a to ac07097 Compare December 8, 2023 11:15

Amaneusz added 14 commits December 8, 2023 15:25

MCPODS-6535 passing storage params to diasble pg_hints

49718b3

MCPODS-6535 wip: debugging emptyCursor

995f6c7

MCPODS-6535 or fix

27c203d

MCPODS-6535 igoring potentially wrong error handling, passing tests: …

552de40

…38/51

MCPODS-6535 fixed couple of test cases, debugging lost connection / p…

feeb9e3

…kill on pg instance in test 401 & following

MCPODS-6535 post-debug clean-ups

619185a

MCPODS-6535 review fixes

947ae58

MCPODS-6535 mock fix

973a389

MCPODS-6535 fixed collection-does-not-exist code; facing Psql constru…

9d43091

…ctor issue (PluginCache)

MCPODS-6535 closing results, debugging PluginCache in progress

c89dc68

MCPODS-6535 refactored PluginCache storage-related code, fixed Positi…

c408e34

…on bug

MCPODS-6535 fixed PsqlCursor (added close method impl)

120b802

MCPODS-6535 openapi update

fb18c35

MCPODS-6535 fixed most tests except wiring (to be addressed - psql se…

2a5b3d4

…tup)

Amaneusz force-pushed the MCPODS-6535-validate_rest_against_psql branch from ac07097 to 7ed7f2f Compare December 8, 2023 14:31

MCPODS-6535 wiring test fix(?) & test db setup

0b8c67f

Amaneusz force-pushed the MCPODS-6535-validate_rest_against_psql branch 2 times, most recently from dcd0280 to 776d71e Compare December 8, 2023 14:56

MCPODS-6535 auth fix

4b95b47

Amaneusz force-pushed the MCPODS-6535-validate_rest_against_psql branch from 776d71e to 4b95b47 Compare December 8, 2023 15:00

hirenkp2000 requested changes Dec 8, 2023

View reviewed changes

MCPODS-6535 auth fix

d857770

hirenkp2000 approved these changes Dec 8, 2023

View reviewed changes

Amaneusz and others added 4 commits December 8, 2023 16:26

MCPODS-6535 pr remarks

5030704

MCPODS-6535 handling missing conf

2c85ff6

Removing DB part for NakshaHubWiring test

65702f9

reduced lib-hub coverage threshold

a3d3a8a

Fixed local start of NakshaApp and removed default storage entry for …

1a72d22

…"psql"

added rollback for error situation

3b616b6

hirenkp2000 merged commit a396744 into Naksha_maintenance Dec 8, 2023
2 checks passed

hirenkp2000 deleted the MCPODS-6535-validate_rest_against_psql branch December 12, 2023 08:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mcpods 6535 validate rest against psql #112

Mcpods 6535 validate rest against psql #112

Amaneusz commented Nov 30, 2023

github-actions bot commented Nov 30, 2023

github-actions bot commented Nov 30, 2023

hirenkp2000 left a comment

hirenkp2000 Dec 1, 2023

Amaneusz Dec 1, 2023

hirenkp2000 Dec 1, 2023

Amaneusz Dec 1, 2023

hirenkp2000 Dec 1, 2023

Amaneusz Dec 1, 2023

hirenkp2000 Dec 1, 2023

hirenkp2000 Dec 1, 2023

github-actions bot commented Dec 1, 2023

hirenkp2000 left a comment •

edited

Loading

hirenkp2000 Dec 8, 2023

hirenkp2000 Dec 8, 2023

hirenkp2000 Dec 8, 2023

hirenkp2000 Dec 8, 2023

github-actions bot commented Dec 8, 2023

github-actions bot commented Dec 8, 2023

github-actions bot commented Dec 8, 2023

Mcpods 6535 validate rest against psql #112

Mcpods 6535 validate rest against psql #112

Conversation

Amaneusz commented Nov 30, 2023

github-actions bot commented Nov 30, 2023

Code Coverage

github-actions bot commented Nov 30, 2023

Code Coverage

hirenkp2000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 1, 2023

Code Coverage

hirenkp2000 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 8, 2023

Code Coverage

github-actions bot commented Dec 8, 2023

Code Coverage

github-actions bot commented Dec 8, 2023

Code Coverage

hirenkp2000 left a comment •

edited

Loading