Skip to content
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

sap-language parameter is not added to sap.ui.model.odata.v2.ODataModel if model is extended #3126

Closed
iljapostnovs opened this issue Jan 12, 2021 · 34 comments
Assignees

Comments

@iljapostnovs
Copy link

OpenUI5 version: 1.85.0

Language url parameter is not added to the ODataModel if model is extended.
Steps to reproduce the problem:

  1. Create custom ODataModel by extending sap.ui.model.odata.v2.ODataModel (I believe that same issue appears for v4)
  2. Add model to the manifest.json

What is the expected result?
sap-language url parameter should be added

What happens instead?
sap-language url parameter is not added

Any other information? (attach screenshot if possible)
Reason for it is in sap.ui.core.Component line: 1545
image
sap-language should be added if the model is instance of ODataModel, however right now class check is strictly set to sap.ui.model.odata.v2.ODataModel and sap.ui.model.odata.v4.ODataModel models.

@flovogt
Copy link
Member

flovogt commented Jan 13, 2021

Hi @iljapostnovs,

Thank you for sharing this finding. I've created an internal incident 2180022569. The status of the issue will be updated here in GitHub.

All the best,
Florian

@flovogt
Copy link
Member

flovogt commented Jan 14, 2021

Hi @iljapostnovs,
I had a look on the respective code lines and it's not so easy how it looks like at the first glance.
At this point, the models are not yet generated. So, no model instance is available to perform a "isA" to check for inheritance.
Our recommendation is to override your component implementation and add the needed code.
Best Regards,
Florian

@iljapostnovs
Copy link
Author

Hi,

_createManifestModelConfigurations method is private, according to OOP principles you can't override private methods.

Parameters for the model could be set after model creation, it's line 2884 in sap.ui.core.Component:
image

@iljapostnovs
Copy link
Author

iljapostnovs commented Jan 14, 2021

Or actually it's not so easy, because metadata might be requested without language parameter anyway.

In this case the model class can be requested using sap.ui.requireSync right before setting the parameters, and then you can use isA. It looks like drawback because require should be async, but taking in mind requireSync is used anyway in _createManifestModels, it will impact nothing.

@flovogt
Copy link
Member

flovogt commented Jan 15, 2021

Hi,

_createManifestModelConfigurations method is private, according to OOP principles you can't override private methods.

Parameters for the model could be set after model creation, it's line 2884 in sap.ui.core.Component:
image

Hi @iljapostnovs, yes the method is private. But as of now, this would be the only option for you to achieve your scenario.

@flovogt
Copy link
Member

flovogt commented Jan 15, 2021

Or actually it's not so easy, because metadata might be requested without language parameter anyway.

In this case the model class can be requested using sap.ui.requireSync right before setting the parameters, and then you can use isA. It looks like drawback because require should be async, but taking in mind requireSync is used anyway in _createManifestModels, it will impact nothing.

Thanks a lot for your inputs. We'll discuss your points and report the outcome here 👍🏽

@iljapostnovs
Copy link
Author

iljapostnovs commented Jan 17, 2021

Hi,
_createManifestModelConfigurations method is private, according to OOP principles you can't override private methods.
Parameters for the model could be set after model creation, it's line 2884 in sap.ui.core.Component:
image

Hi @iljapostnovs, yes the method is private. But as of now, this would be the only option for you to achieve your scenario.

Thanks, but I figured out another temporary way out:

sap.ui.define([
	"sap/ui/model/odata/v2/ODataModel"
], function (ODataModel) {
	"use strict";

	return ODataModel.extend("com.test.ODataModel", {
		constructor: function() {
			this._addSAPLanguageToUrlParameters(arguments);

			return ODataModel.prototype.constructor.apply(this, arguments);
		},

		_addSAPLanguageToUrlParameters: function(aArgs) {
			const oConfig = sap.ui.getCore().getConfiguration();
			const mSettings = aArgs[1];
			if (mSettings && typeof mSettings === "object") {
				mSettings.metadataUrlParams = mSettings.metadataUrlParams || {
					"sap-language": oConfig.getSAPParam("sap-language")
				};
			}
		}
	});
});

Waiting forward getting rid of this :)

@flovogt
Copy link
Member

flovogt commented Jan 21, 2021

Hi @iljapostnovs,
thanks a lot for providing the code snippet also for other developers. We had a deeper look at your requirement and we definitely prefer your solution.
Requiring the models in the component synchronously is not good because there are applications not using ODataModels at all.
While using the approach in your latest code snippet you'll be fine also for future releases.
Thanks a lot for opening this issue and don't hesitate to open further ones in future. We're looking forward to any feature request or idea by the community.
All the best and take care,
Florian

@iljapostnovs
Copy link
Author

iljapostnovs commented Jan 21, 2021

Hi @iljapostnovs,
thanks a lot for providing the code snippet also for other developers. We had a deeper look at your requirement and we definitely prefer your solution.
Requiring the models in the component synchronously is not good because there are applications not using ODataModels at all.
While using the approach in your latest code snippet you'll be fine also for future releases.
Thanks a lot for opening this issue and don't hesitate to open further ones in future. We're looking forward to any feature request or idea by the community.
All the best and take care,
Florian

Hi,
Fair.
I could propose to move language parameter logic from Component class to ODataModel class, I don't see why language parameter should be set only if you declared ODataModel in manifest :)

@iljapostnovs
Copy link
Author

iljapostnovs commented Jan 21, 2021

Actually I thought a bit about your answer and didn't get the problem.
Models are created using requireSync anyway in _createManifestModels method, which means that if you require the class earlier - it will change nothing.

Requiring the models in the component synchronously is not good because there are applications not using ODataModels at all.

That's of couse true, but I don't see how it is a problem.
Lets simulate some scenarios:

  1. Class com.test.ODataModel is created, which extends sap.ui.model.v2.ODataModel.
    com.test.ODataModel class is preloaded and .isA("sap.ui.model.v2.ODataModel") is used. It returns true, the parameters are added and earlier preloaded class is used later on in _createManifestModels method for creating the instance of the model.

  2. Class com.test.JSONModel is created, which extends sap.ui.model.json.JSONModel.
    com.test.JSONModel class is preloaded and .isA("sap.ui.model.v2.ODataModel") is used. It returns false, the parameters are not added and earlier preloaded class is used later on in _createManifestModels method for creating the instance of the model.

As far as I understood from your answer, the problem is that you need to require ODataModel in scenario 2, but according to .isA implementation that's not actually true:
image
.isA is not preloading the class which is used for checking.

@codeworrior
Copy link
Member

Hi @iljapostnovs.

Models are created using requireSync anyway in _createManifestModels method, which means that if you require the class earlier - it will change nothing.

This is unfortunately not true. The point in time where the models now are required and loaded is guaranteed to have preloads the code already. For the earlier point in time where the model settings are prepared (incl. the injection of the sap-language code), this is not guaranteed and therefore sync XHRs could be created for individual files. The path on which models that are flagged with "preload:true" are loaded, explicitly checks whether the code is preloaded already and if not, postpones model creation.

.isA is not preloading the class which is used for checking.

This is correct (and the reason why we introduced .isA), but only for the target of the comparison. But the method can only be called on class metadata or on a class instance. Both require that the class has been required and executed beforehand. Therefore, the component still would have to require com.test.JSONModel early and due to the above timing issue, we have to avoid this.

We could optimise the check for some scenarios (mainly for apps that use a self-contained bundle): If the model implementation has already been preloaded, then we could require it and do the isA check instead of just comparing its name against a list of well known implementations. However, I don't think this would be a good solution as it would only work for certain deployments of an app whereas for others, the parameters would not be injected.

I could propose to move language parameter logic from Component class to ODataModel class, I don't see why language parameter should be set only if you declared ODataModel in manifest :)

That's the most appealing alternative from my POV. I think the reason why it originally wasn't done like that was that the models don't have access to some of the parameters (esp. cache tokens). The global settings like language and client however would be available to them.

@iljapostnovs
Copy link
Author

for apps that use a self-contained bundle

Hi @codeworrior,
Thanks for explanation.
I would be more than happy to see the url parameter in ODataModel instead of Component :)
Should I create new issue for it?

@ThomasChadzelek
Copy link
Member

I don't fully get the goal we are after here. The V4 OData model is already using "Accept-Language" header automatically. My understanding is that the sap-language URL parameter is most valuable when it comes to caching, and as @codeworrior said, "the models don't have access to some of the parameters (esp. cache tokens)". So what is the benefit of having sap-language URL w/o the others?

@iljapostnovs
Copy link
Author

I don't fully get the goal we are after here. The V4 OData model is already using "Accept-Language" header automatically. My understanding is that the sap-language URL parameter is most valuable when it comes to caching, and as @codeworrior said, "the models don't have access to some of the parameters (esp. cache tokens)". So what is the benefit of having sap-language URL w/o the others?

Hi,
sap-language is valuable not only for caching.
Real life example:

  1. Create com.test.ODataModel which extends sap.ui.model.odata.v2.ODataModel
  2. Bind labels to ODataModel metadata labels.
  3. You have e.g. German language in your Fiori Launchpad, but system language is EN. You will get english labels instead of german anyway, because sap-language parameter is added automatically only for sap.ui.model.odata.v2.ODataModel, for com.test.ODataModel it will not be added.

@ThomasChadzelek
Copy link
Member

OK, I think I understand your scenario. As I said, this should not be an issue with v4.ODataModel because "Accept-Language" header is automatically set for both data and metadata. Just to clarify the scope of this issue...

@iljapostnovs
Copy link
Author

OK, I think I understand your scenario. As I said, this should not be an issue with v4.ODataModel because "Accept-Language" header is automatically set for both data and metadata. Just to clarify the scope of this issue...

I didn't test this scenario for V4 ODataModel, so I hope that you are right :)

@ThomasChadzelek
Copy link
Member

Looking at the code, I'd say that V2 is also using "Accept-Language" headers for both data and metadata. Can you please confirm this? Why doesn't this have the same effect as sap-language?

@iljapostnovs
Copy link
Author

iljapostnovs commented Jan 26, 2021

Looking at the code, I'd say that V2 is also using "Accept-Language" headers for both data and metadata. Can you please confirm this? Why doesn't this have the same effect as sap-language?

Hi,

Here is metadata request without sap-language parameter in LT language:
image
And here is english label:
image

Here is metadata request with sap-language parameter in LT language:
image
And here is lithuanian label:
image

So I guess at least v2 ODataModel is not using accept-language header.

@ThomasChadzelek
Copy link
Member

Hello @iljapostnovs !

Thanks for this detailed analysis. When I speak of ODataModel, I refer to UI5's implementation. Your screenshot illustrates the presence of "Accept-Language" request headers. But I also understand that the server's response treats "sap-language" differently than "Accept-Language". That is not a question to UI5 in the 1st place. Which server implementation/framework are you using?

Best regards,
Thomas

@iljapostnovs
Copy link
Author

Hello @iljapostnovs !

Thanks for this detailed analysis. When I speak of ODataModel, I refer to UI5's implementation. Your screenshot illustrates the presence of "Accept-Language" request headers. But I also understand that the server's response treats "sap-language" differently than "Accept-Language". That is not a question to UI5 in the 1st place. Which server implementation/framework are you using?

Best regards,
Thomas

I am using NetWeaver OData Service. I don't know which component exactly is responsible for OData implementation, but my guess would be that it is SAP_GWFND, release 750, SP-level 0008.
Indeed, the NW OData is not related to UI5, but adding sap-language url parameter to the request is. Taking in mind that parameter is added in the sap.ui.core.Component implementation, I hope that it makes sense to add it for all ODataModel (model extensions as well) instances, not only strictly for sap.ui.model.odata.v2.ODataModel models.

@ThomasChadzelek
Copy link
Member

Hello @iljapostnovs !

Thanks for your input. I will discuss this internally (beginning of next week) and let you know here. Stay tuned!

Best regards,
Thomas

@ThomasChadzelek
Copy link
Member

Hello @iljapostnovs !

I've got at least a preliminary answer: "Fix the server". I suggest you try and open a support ticket on SAP_GWFND asking why the "Accept-Language" request header does not have the desired effect. Then we will see if UI5 needs to change anything at all.

Best regards,
Thomas

@iljapostnovs
Copy link
Author

Hello @iljapostnovs !

I've got at least a preliminary answer: "Fix the server". I suggest you try and open a support ticket on SAP_GWFND asking why the "Accept-Language" request header does not have the desired effect. Then we will see if UI5 needs to change anything at all.

Best regards,
Thomas

Hi,

I googled a bit about accept-language and seems that it works correctly.
https://help.sap.com/viewer/17ae0e97e0fc424a9c368f350c0ba6bd/2.10/en-US/65edb2db3a6b452a97633f0027bf0922.html

  1. sap-language is checked
  2. If no sap-language is provided, user language (SU01) is checked
  3. If no user language detected, accept-language header is checked

In my case the default system language is EN, but users can login with different language.
If sap-language is not provided, language from SU01 is taken, and it is EN.
This basically means, that without sap-language URL parameter user will always get EN labels, no matter what language is used by Fiori Launchpad.

@ThomasChadzelek
Copy link
Member

Is that a generic user you are using? Would it be an option to set the desired language also in SU01? But I get the point that Fiori Launchpad settings should win...

@iljapostnovs
Copy link
Author

Is that a generic user you are using?

At least that's the case for my user in NW system I have.

Would it be an option to set the desired language also in SU01?

Technically yes, but I don't think that somebody will be happy to edit thousands of user languages. And again, that also basically means that metadata language will be strictly set to one language, which should not be the case.

But I get the point that Fiori Launchpad settings should win...

Precisely

@ThomasChadzelek
Copy link
Member

Hello @iljapostnovs !

Could you please do us a favor and try your scenario with a different language, maybe "en" or "de" or "fr" or "es" or "it"? That might make a difference and would be interesting to know.

Best regards,
Thomas

@iljapostnovs
Copy link
Author

Hello @iljapostnovs !

Could you please do us a favor and try your scenario with a different language, maybe "en" or "de" or "fr" or "es" or "it"? That might make a difference and would be interesting to know.

Best regards,
Thomas

Hi,

What exactly you want me to try out?
Change sap-language, accept-language or user language in SU01?

@ThomasChadzelek
Copy link
Member

Hello @iljapostnovs !

Good point, I should have made this clearer. My understanding is that you start your UI5 app in a way which tells UI5 that Lithuanian is your language of choice. From the above, it sounds like Fiori Launchpad is where you maintain that setting. We have already seen that the Accept-Language request header is properly influenced by that setting, but it seemed to be ignored by the backend. My suggestion is now to use maybe "en" or "de" or "fr" or "es" or "it" as you language of choice in Fiori Launchpad. That should influence the Accept-Language request header and maybe make a difference for the server.

Best regards,
Thomas

@iljapostnovs
Copy link
Author

Hello @iljapostnovs !

Good point, I should have made this clearer. My understanding is that you start your UI5 app in a way which tells UI5 that Lithuanian is your language of choice. From the above, it sounds like Fiori Launchpad is where you maintain that setting. We have already seen that the Accept-Language request header is properly influenced by that setting, but it seemed to be ignored by the backend. My suggestion is now to use maybe "en" or "de" or "fr" or "es" or "it" as you language of choice in Fiori Launchpad. That should influence the Accept-Language request header and maybe make a difference for the server.

Best regards,
Thomas

Hi,

I have tried to do the same with ET, result is the same.
W/O sap-language param:
image
image

With sap-language param:
image
image

@ThomasChadzelek
Copy link
Member

Hello @iljapostnovs !

I just learned that our concerns regarding a difference between "lt" and "en" was kind of a false alarm. Sorry for that!

Our current thinking is that Accept-Language is being ignored because you have already established a session with your backend (browser cookies) and that session's language wins. In that scenario, we would expect sap-language to NOT make any difference. Your screenshots show "standalone" requests in a browser where not session has been established, I guess. If you use a request with sap-language in a browser tab where your app is already running and fetching the wrong metadata - would that really make a difference?

Please apologize our confusion, but there is some uncertainty as to the order of factors which influence a language, so to say.

Best regards,
Thomas

@iljapostnovs
Copy link
Author

Hello @iljapostnovs !

I just learned that our concerns regarding a difference between "lt" and "en" was kind of a false alarm. Sorry for that!

Our current thinking is that Accept-Language is being ignored because you have already established a session with your backend (browser cookies) and that session's language wins. In that scenario, we would expect sap-language to NOT make any difference. Your screenshots show "standalone" requests in a browser where not session has been established, I guess. If you use a request with sap-language in a browser tab where your app is already running and fetching the wrong metadata - would that really make a difference?

Please apologize our confusion, but there is some uncertainty as to the order of factors which influence a language, so to say.

Best regards,
Thomas

Hi,

Sorry for the delay on my answer, I was occupied by other tasks.
Indeed, I was testing using local environment with EN session established and I was changing language using sap-language parameter.
I have deployed the custom ODataModel to FLP without adding sap-language parameter and tried to login in another language and it worked for me, however I had a case when language was still working incorrectly in FLP.
I will get back to you when I remember how to reproduce the issue in FLP.

However, If I will not be able to reproduce the issue, I'm still thinking that it's incorrect behavior that adding sap-language parameter affects everything except custom ODataModel even if I'm working with local development environment.

@Shtilianova
Copy link
Contributor

I've created an internal incident 2170257172. The status of the issue will be updated here in GitHub.

@codeworrior
Copy link
Member

Triggered by this issue, we discussed again whether we can officially support custom subclasses of the existing models and finally decided against it. The API documentation now (db7069f) states for JSONModel, MessageModel, ODataModel (v1,v2,v4), ODataMetaModel, ResourceModel and XMLModel

This model is not prepared to be inherited from.

You might notice that "...is not prepared..." is a rather soft statement. Technically, we do not prevent subclassing (model classes are not declared to be final). We just have no mechanism or documentation in place which would help subclasses to avoid incompatibilities (e.g. naming collisions if subclasses introduce new methods; or contract issues if subclasses override inherited methods with a certain contract in mind that we then change; or timing issues when the call sequence for certain methods/hooks/events within a model implementation changes etc.).

Especially in cloud scenarios with automatic version upgrades, there's no point in time where such incompatibilities could be detected and resolved before they break productive usages.

That's why we thought it would be fair to document this as a restriction / warning.

Well, at the same time, we re-implemented the model handling in the Component.js (2183384). It is now no longer based on class name equality, but uses the isA functionality. isA works similar to instanceof, but does not require a hard dependency and also takes UI5 interfaces into account.

@iljapostnovs
Copy link
Author

Triggered by this issue, we discussed again whether we can officially support custom subclasses of the existing models and finally decided against it. The API documentation now (db7069f) states for JSONModel, MessageModel, ODataModel (v1,v2,v4), ODataMetaModel, ResourceModel and XMLModel

This model is not prepared to be inherited from.

You might notice that "...is not prepared..." is a rather soft statement. Technically, we do not prevent subclassing (model classes are not declared to be final). We just have no mechanism or documentation in place which would help subclasses to avoid incompatibilities (e.g. naming collisions if subclasses introduce new methods; or contract issues if subclasses override inherited methods with a certain contract in mind that we then change; or timing issues when the call sequence for certain methods/hooks/events within a model implementation changes etc.).

Especially in cloud scenarios with automatic version upgrades, there's no point in time where such incompatibilities could be detected and resolved before they break productive usages.

That's why we thought it would be fair to document this as a restriction / warning.

Well, at the same time, we re-implemented the model handling in the Component.js (2183384). It is now no longer based on class name equality, but uses the isA functionality. isA works similar to instanceof, but does not require a hard dependency and also takes UI5 interfaces into account.

That's actually one of my proposals of solving the problem and exactly what was needed, thanks!

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

No branches or pull requests

5 participants