Remove circular dependency between entity and api schema

[gfakbar20]

This pr is made to remove the direct dependency of API schema in the entity class as mentioned in #780

[eric-maynard]

Not a big fan of the static imports outside of test code

[eric-maynard]

Overall this looks like a good change to me. I have one doubt after reading #780 though -- is it possible to actually remove the dependency after this change? As in, the gradle dependency?

[dimas-b]

Does this actually remove the circular dependency? The old CatalogEntity.asCatalog() method and this method do the same thing and exist in the case package 🤔

[flyrain]

I didn't see any circular dependency in the code base. Here is the current dependency tree

1. polaris-api-management-model depends no one
2. polaris-core depends on polaris-api-management-model
3. polaris-api-management-service depends on polaris-api-management-model and polaris-core.

[snazy]

This change is a very good step to remove the dependency of (low level) persistence concerns to (high level) API concerns, which can be, as you mentioned in the PR summary, considered a circular dependency. I do support this change.

[dimas-b]

@gfakbar20 : your contribution is still valuable, but may need a bit extra work, I guess... If you feel like continuing this effort, I'd propose starting a dev email discussion, so as to clarify the location of the refactored code.

[gfakbar20]

Hi @dimas-b , after i took a closer look of the code last night, the complete removal of the gradle dependency will make this PR scope to huge. Hence i:

• will reopen this PR soon (but the polaris-core will still dependent on polaris-management-api-model)
• I think i need to make a POC before opening a new thread in the mailing list

Best regards,
Ginda

[gfakbar20]

and no, i do not intend to permanently closed this pr as I believe @snazy concern lead to a bigger issue.

[snazy]

The overall concern I raised in #780 is that the low-level persistence model depends on higher-level public API types.
So this change to remove the direct dependencies from the persistence model types is a good first step towards decoupling it. @dimas-b's comment, AFAIU was that the EntityConverter should live in a package in or near the public API code.

Refactoring the code base wrt Gradle modules wasn't explicitly being asked in #780. But that's a way bigger effort.

[flyrain]

PolarisEntityCore and its derived classes serve as business objects shared across multiple persistence backends. They are not persistence model types themselves. I’m open to relocating the converters if needed, but could you clarify the concern around placing converters within business object classes?

In general, models defined in the spec are more stable than business or persistence models. It's typically acceptable for those models to depend on stable entities, but not the other way around.

Also, when you mentioned circular dependencies, are we talking about actual runtime dependency issues or something specific to Gradle module boundaries? If it's not about Gradle, what kind of circular dependency are we concerned about here?

[eric-maynard]

+1 @flyrain, your comment reminds me of the discussion on this other PR. Unfortunately right now the ...Entity... types are not purely persistence concerns. It might be nice if that were the case, but that would require introduction of some new business types (or DAOs) first.

However, I don't think having converters is the worst idea, at least it will centralize some of the management of these objects and allow for future improvements like the introduction of new intermediary types.

[snazy]

One important aspect is overlooked here: Persistence implementations have to deal with these entities, so the low-level implementations have to depend on these high-level types, and also object storage and other stuff. So decoupling all that from persistence concerns is beneficial, and so is this effort.

[gfakbar20]

I’m open to relocating the converters if needed, but could you clarify the concern around placing converters within business object classes?

just a pragmatic one, and the current usage is inside the polaris service common module since I'm not so sure about the source code progression. But placing it in the public api model module sounds good to me because of the clearer separation and better module ownership.

Also, when you mentioned circular dependencies, are we talking about actual runtime dependency issues or something specific to Gradle module boundaries? If it's not about Gradle, what kind of circular dependency are we concerned about here?

it tends toward runtime dependency because the service implementation is injected by quarkus. I will use create catalog as an example. The flow will be as follows:

• Catalog is used as an implicit parameter in PolarisCatalog.createCatalog method.
• PolarisCatalog.createCatalog calls PolarisCatalogApiService.createCatalog using the same object class which will be implemented by PolarisServiceImpl.createCatalog.
• Inside PolarisServiceImpl.createCatalog, the converter method inside CatalogEntity will transform an object from or to between Api payload schema (Catalog) and the business model (CatalogEntity).

Well it is nice to know that the API will be stable, but is the openApi generator also stable enough to be updated by the dependabot? I only worked with openApi generator shortly 2 years ago to notice any detrimental(breaking) changes to my project when the version is upgraded.

[gfakbar20]

if no, think it is a good idea to localized the usage of openApi generated class instead of the widespread use of it as currently happen

[flyrain]

But placing it in the public api model module sounds good to me because of the clearer separation and better module ownership.

Placing the converter in the api model module will make a more stable module(API model) depends on a less stable one(Polaris core). I think that's an anti-pattern we will need to avoid.

Inside PolarisServiceImpl.createCatalog, the converter method inside CatalogEntity will transform an object from or to between Api payload schema (Catalog) and the business model (CatalogEntity).

I’m not quite following why this is considered a circular dependency, could you elaborate? The converter is simply responsible for transforming between a business object and an API model, and it fits naturally within the module where the business object resides.

I do agree with @eric-maynard that centralizing model converters could be a good idea for maintainability and consistency. But that feels like a design choice, not something related to circular dependency.

Well it is nice to know that the API will be stable, but is the openApi generator also stable enough to be updated by the dependabot? I only worked with openApi generator shortly 2 years ago to notice any detrimental(breaking) changes to my project when the version is upgraded.

I don’t think the OpenAPI generator should be a concern in this case. Regardless of how it's generated, API models are fundamentally more stable than business or persistence entities. Even in the worst-case scenario, we could always fall back to manually adding the API models.

[flyrain]

Looking a bit more, the only reason that polaris-api-management-service depends on polaris-core is that polaris-api-management-model misses the concept of RealmContext. Maybe we should try to add the RealmContext to the REST spec.

[snazy]

The type RealmContext has actually nothing to do with the API itself. The right way would be to @Inject RealmContext where it is needed. That seems cleaner than moving types.

[gfakbar20]

I don’t think the OpenAPI generator should be a concern in this case. Regardless of how it's generated, API models are fundamentally more stable than business or persistence entities. Even in the worst-case scenario, we could always fall back to manually adding the API models.

stable because of the API spec or we are sure of the generator's behavior relatives to the API spec, meaning that regardless of which openapi version it will not generate a technically different class?

[snazy]

@gfakbar20 I second your general concern about OpenAPI code generators, as I've seen them generating wrong code in the past as well. OTOH, we have never experienced issues with spec files generated from/using the Java Microprofile OpenAPI annotations.

[flyrain]

stable because of the API spec or we are sure of the generator's behavior relatives to the API spec, meaning that regardless of which openapi version it will not generate a technically different class?

API spec changes affect the business and persistence entities, not the other way around. These are independent of the generator. In the worst-case scenario, we can always create the API models manually.

[flyrain]

The type RealmContext has actually nothing to do with the API itself. The right way would be to @Inject RealmContext where it is needed. That seems cleaner than moving types.

That's one option. There could be other options. Can we bring the discussion to dev ML?