feat: Remove duplicate information in the initiate negotiation api request

[tuncaytunc-zf]

What this PR changes/adds

Removes the duplicate information in the initiate negotiation api request

Why it does that

The offerId and the policy/@id have the same value, because they're representing the same information, so there's no point in offerId, as the policy is mandatory anyway.

Further notes

This introduces new api version of negotiation to avoid any braking changes

Linked Issue(s)

Closes #3395

Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.

[codecov-commenter]

Codecov Report

Attention: 68 lines in your changes are missing coverage. Please review.

Comparison is base (e30ca41) 72.45% compared to head (9c43497) 18.16%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #3549       +/-   ##
===========================================
- Coverage   72.45%   18.16%   -54.30%     
===========================================
  Files         871      877        +6     
  Lines       17423    17566      +143     
  Branches      993      995        +2     
===========================================
- Hits        12624     3190     -9434     
- Misses       4382    14289     +9907     
+ Partials      417       87      -330     

Files	Coverage Δ
...nt/contractnegotiation/ContractNegotiationApi.java	0.00% <ø> (ø)
...tnegotiation/ContractNegotiationApiController.java	100.00% <ø> (ø)
...gotiation/v3/ContractNegotiationApiController.java	100.00% <100.00%> (ø)
...ctnegotiation/ContractNegotiationApiExtension.java	0.00% <0.00%> (-92.86%)	⬇️
...ion/v3/validation/ContractRequestDtoValidator.java	0.00% <0.00%> (ø)
...contractnegotiation/v3/ContractNegotiationApi.java	0.00% <0.00%> (ø)
...ntractnegotiation/v3/model/ContractRequestDto.java	65.62% <65.62%> (ø)
...ontractRequestDtoToContractRequestTransformer.java	0.00% <0.00%> (ø)
...orm/JsonObjectToContractRequestDtoTransformer.java	0.00% <0.00%> (ø)

... and 568 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ndr-brt]

I think there are two ways of doing this.
The way you followed brings in a Dto class, that we decided to remove some weeks ago.
Plus, a new Dto requires a two steps transformation (json -> dto -> model) that we don't really want.

I see that in this case they are needed because there is no way to define different validators and transformers for the same entity based on the controller they are used.

A new api version should be motivated for substantial changes, and I think the changes here are really little and they can be handled in the same version, as we did in the past in other APIs following this approach:

• deprecate the properties on the schema
• broaden the validator to accept the deprecated field as optional
• in the transformer, check if the deprecated properties are used, in that case, print a warning log

to get a grasp, we did the same thing when we renamed providerUrl to counterPartyAddress in the CatalogRequest, please look at JsonObjectToCatalogRequestTransformer.

[tuncaytunc-zf]

I think there are two ways of doing this. The way you followed brings in a Dto class, that we decided to remove some weeks ago. Plus, a new Dto requires a two steps transformation (json -> dto -> model) that we don't really want.

I see that in this case they are needed because there is no way to define different validators and transformers for the same entity based on the controller they are used.

A new api version should be motivated for substantial changes, and I think the changes here are really little and they can be handled in the same version, as we did in the past in other APIs following this approach:

• deprecate the properties on the schema
• broaden the validator to accept the deprecated field as optional
• in the transformer, check if the deprecated properties are used, in that case, print a warning log

to get a grasp, we did the same thing when we renamed providerUrl to counterPartyAddress in the CatalogRequest, please look at JsonObjectToCatalogRequestTransformer.

After a short discussion with @wolf4ood we decided to go for a new Dto that's why I have implemented it like this. Unfortunately I was not aware about such a decision probably Enrico neither. But indeed introducing a new api version for such a small change would cause inconvenience. I will close this PR and implement the second way you described.

[wolf4ood]

My two cents, it's fine to supports different fields on the same Object,
but in this case IMHO the shape of the input object changes substantially and the single transformer would basically handle two different shapes in one .
Having the DTO in the middle it's something that it's not wanted but in this case would be only a shim for the time being until the v2 is deprecated.

We did the same with Asset AP, by changing the shape cause the v3 :)

[ndr-brt]

I'd propose compromise then: how do you see to add the Dto for the deprecated endpoint and have the new one without it? In that case the Dto will be removed when the deprecation will expire

[wolf4ood]

For me it's fine also to have it in a deprecation way, as long as it will be deprecated soon 😂
and we should at least document well the behaviour of the combination of assetId + policy vs offer.

But i guess at this point as you wrote we could remove also the assetId and just use the policy?

In that case I'm fine with supporting only the policy field in addition and mark for deprecation offer :)

[tuncaytunc-zf]

If dto comes into consideration what would help to not frequently change the core model whenever an api schema changes, why then not keep it like implemented here and remove it when it gets deprecated instead of changing it now and remove it afterward 😂

[ndr-brt]

If dto comes into consideration what would help to not frequently change the core model whenever an api schema changes, why then not keep it like implemented here and remove it when it gets deprecated instead of changing it now and remove it afterward 😂

because when someone takes care of removing deprecated stuff they just want to delete stuff that's marked as deprecated without having to do stuff that requires specific knowledge.

But i guess at this point as you wrote we could remove also the assetId and just use the policy?

In that case I'm fine with supporting only the policy field in addition and mark for deprecation offer :)

I agree with that, also the assetId field is not necessary as it is already pointed out by the policy (offer would be a better name for it but unfortunately it's already taken).

So, having an optional policy (or contractOffer or whathever) field instead of the deprecated offer would already solve the issue, without v3, dtos, additional validators and transformers.

[wolf4ood]

So, having an optional policy (or contractOffer or whathever) field instead of the deprecated offer would already solve the issue, without v3, dtos, additional validators and transformers.

👍

[github-actions]

This pull request is stale because it has been open for 7 days with no activity.