SAP / openui5

OpenUI5 lets you build enterprise-ready web applications, responsive to all devices, running on almost any browser of your choice.
http://openui5.org
Apache License 2.0
2.96k stars 1.24k forks source link

Hardcoded namespace for on read attributes with "sap:" prefix (Annotations for OData Version 2.0) #1560

Closed renanwilliam closed 7 years ago

renanwilliam commented 7 years ago

OpenUI5 version: 1.46.10

Browser/version (+device/version): PC/Chrome

Any other tested browsers/devices(OK/FAIL): N/A

URL (minimal example if possible): N/A

Steps to reproduce the problem:

  1. Define a custom "sap" namespace using metadataNamespaces in sap.ui.model.odata.v2.ODataModel constructor like that
{
   "edmx": "http://schemas.microsoft.com/ado/2007/06/edmx",
   "atom": "http://www.w3.org/2005/Atom",
   "sap" : "http://www.successfactors.com/edm/sap",
   "m" : "http://schemas.microsoft.com/ado/2007/08/dataservices/metadata",
   "sf" : "http://www.successfactors.com/edm/sf",
   "": "http://schemas.microsoft.com/ado/2008/09/edm"
}
  1. Setup the source for service as SuccessFactors OData API : https://sfsfbizxtrial.hana.ondemand.com/odata/v2/$metadata (s-user or p-user required) - it uses a alternative namespace for "sap:" annotations = xmlns:sap="http://www.successfactors.com/edm/sap"
  2. As the namespace is hardcoded at https://github.com/SAP/openui5/blob/master/src/sap.ui.core/src/sap/ui/model/odata/_ODataMetaModelUtils.js#L856 the label properties isn't loaded correctly

What is the expected result? The framework need to respect the namespaces defined in metadataNamespaces parameter sent in class constructor

What happens instead? The framework no respect the namespaces defined in metadataNamespaces parameter, using a hard coded value.

Any other information? (attach screenshot if possible) It have impacts for SmartControls that not get the correct labels in SuccessFactors Extensions (I know isn't OpenUI5 scope but as the code is shared with SAPUI5 I think is appropriated discuss it here too)

ThomasChadzelek commented 7 years ago

Hello Renan!

I'm sorry, but ODataMetaModel took the decision to map the hard-coded Namespace "http://www.sap.com/Protocols/SAPData" (in XML) to the shortcut "sap:" (in JSON). I don't see that this can be changed after all These years, in a compatible way.

What exactly is the reason why you Need to define another Namespace with the alias "sap" in XML and why exactly does it lead to an issue? Because in JSON, there should be no other properties named "sap:label" etc. Is it because you expect that "http://www.successfactors.com/edm/sap" should Play the same role as "http://www.sap.com/Protocols/SAPData"? Well, that cannot work. In XML, the Namespace is important, not the alias.

Best regards, Thomas

renanwilliam commented 7 years ago

Hi Thomas,

SuccessFactors is a SAP Product and the Data service is designed aligned with the UI5 strategy (these sap:label annotations comes in recents update's). I believe that the proposal of metadataNamespaces parameter is make it dynamic, no make senses have it hard coded - if not is informed the hard coded value will be loaded in https://github.com/SAP/openui5/blob/master/src/sap.ui.core/src/sap/ui/model/odata/ODataMetadata.js#L51

I will try create a pull request for that and a git repo for example.

ThomasChadzelek commented 7 years ago

Hello Renan!

"http://www.sap.com/Protocols/SAPData" is the documented Namespace URL for SAP Annotations for OData Version 2.0 . If you want to use those annotations, why not use exactly that Namespace URL?

Frankly speaking, I do not understand the Code you mention in ODataMetadata.js. The namespace declaration ("mapping of prefix/alias to namespace/URL") is already contained in the XML, e.g. <?xml version="1.0" encoding="utf-8"?>

What is the use Case to override this declaration in a "side channel"? Best regards, Thomas
renanwilliam commented 7 years ago

Hi Thomas,

See the metadata document generated by SuccessFactors: $metadata.xml

I can't change the document namespace because it's generated by SAP SuccessFactors and it's described in the OData API documentation that these annotation are inserted to work exclusive for UI5 controls. In my understanding the namespace needs to come from datasource initialization in constructor of sap.ui.model.odata.v2.ODataModel to make it dynamic to work with another SAP Solutions.

renanwilliam commented 7 years ago

Hi @ThomasChadzelek ,

I've tried to reproduce the scenario but isn't easy. Hope that is understandable. I have created the following repository with 3 branches:

All the three repositories can be execute with grunt default task and accessible in http://localhost:8080/test/mockServer.html

1 - Branch original_version

In this branch I'm trying to use property metadata bind to a text Label using a document with custom sap metadata namespace ( xmlns:sap="http://www.successfactors.com/edm/sap" ) without indicate a custom metadata namespace in datasource definition. As expected, the bind will not work and generate a table with empty headers: image

2 - Branch working_with_property_metadata_bind_expression

In this branch I'm trying to use property metadata bind to a text Label using a document with custom sap metadata namespace ( xmlns:sap="http://www.successfactors.com/edm/sap" ) indicating a custom metadata namespace in datasource definition (in manifest.json). As expected, the bind works and the generated table has the headers filled according with metadata document: image

3 - Branch not_working_with_smartcontrol

In this branch I'm trying to use a SmartControl (SmartTable) using a document with custom sap metadata namespace ( xmlns:sap="http://www.successfactors.com/edm/sap" ) indicating a custom metadata namespace in datasource definition (in manifest.json). The library not work as expected and bind will not work and generate a table with headers using the technical name instead annotations in sap:label in the metadata document: image

My point here is: the namespace can't be placed as hard-code because is already available a parameter in the model constructor to support other SAP products - the second case works properly. I don't know if my pull request (#1592) is written in the best way but the idea is have this namespace read from ODataModel constructor (metadataNamespaces parameter)

ThomasChadzelek commented 7 years ago

Hello @renanwilliam !

Let me 1st answer to your previous comment:

I can't change the document namespace because it's generated by SAP SuccessFactors and it's described in the OData API documentation that these annotation are inserted to work exclusive for UI5 controls. In my understanding the namespace needs to come from datasource initialization in constructor of sap.ui.model.odata.v2.ODataModel to make it dynamic to work with another SAP Solutions.

Your 1st link refers to a page which says "For more information about Extensions Specification in OData, please visit https://scn.sap.com/docs/DOC-44986Information published on SAP site." That page in turn says

The following sections describe which elements of the service document (namespace prefix app) can be annotated with attributes and elements from the namespace http://www.sap.com/Protocols/SAPData (namespace prefix sap) and from the namespace http://www.w3.org/2005/Atom (namespace prefix atom), and what these annotations mean. My understanding of XML is that the Namespace is important, not the prefix you happen to use as an Abbreviation. But then again, I don't get the Point behind the metadataNamespaces Parameter... I will try to understand it better, stay tuned.

Best regards, Thomas

renanwilliam commented 7 years ago

Hi @ThomasChadzelek ,

I think this example repo will be more easier to exemplify the situation. I understand your point of view to keep it hard-coded because is the defined by convention, but definitely makes more sense make it flexible as already works for property metadata bind expressions. SuccessFactors is a SAP product and isn't have the http://www.sap.com/Protocols/SAPData for these properties, indicating that this flexibility is needed. This flexibility is already implemented with metadataNamespaces parameter, is just not used in all points.

ralfhandl commented 7 years ago

In XML namespaces are identified by their URL, see https://www.w3.org/TR/REC-xml-names/.

This means the namespace for SAP extension attributes for OData is identified by the URL http://www.sap.com/Protocols/SAPData, and UI5 is correct in "hard-coding" against this URL.

The short prefix introduced for a namespace URL in the xmlns attribute is a local name that has no meaning beyond being a shorthand local to the element that declares it.

The following two attributes are the same for a correct XML parser:

  <... sap:something="hello" xmlns:sap="http://www.sap.com/Protocols/SAPData" ...>
  <... pro:something="hello" xmlns:pro="http://www.sap.com/Protocols/SAPData" ...>

whereas the following two attributes are completely different for a correct XML parser:

  <... sap:something="hello" xmlns:sap="http://www.sap.com/Protocols/SAPData" ...>
  <... sap:something="hello" xmlns:sap="http://www.successfactors.com/edm/sap" ...>

The problem is with the SuccessFactors service, not with the UI5 client. If the service wants to work with UI5 controls, it has to use the namespace URL http://www.sap.com/Protocols/SAPData.

renanwilliam commented 7 years ago

@ralfhandl Not make any sense keep the namespace hardcoded. Make it default is OK but keep the hardcoded is not a service problem, is a problem with the client library that not have the flexibility. In almost cases we can't change the service - SuccessFactors is a SAP Product and it's a perfect example. Again, the namespaces needs to be configurable, like it already is with metadata property binding - so if it can't change why have it as valid parameter in constructor? why with metadata property binding works?

ThomasChadzelek commented 7 years ago

"metadata property binding" is a different use Case. UI5 allows to write a short path say inside an XML view. You don't want to use the Namespace (URL) there. Thus you can use an alias and provide its Definition elsewhere. But in the end, its the same game: the alias is only of local value, the Namespace is what Counts.

renanwilliam commented 7 years ago

do you have executed my example? probably no but if you look it will see that it don't work without namespace configuration in model constructor.

codeworrior commented 7 years ago

Sorry, we discussed this internally among many developers and architects and we completely disagree.

The namespace for the OData V2 SAP annotations has been standardised for good reasons and using another namespace with the intention of achieving the same result is simply wrong. This can't be fixed by mapping aliases, it either has to be fixed by using the right namespace (favoured approach) or by implementing another mapping layer from the new namespace to one of the standard namespaces or, even better, to V4 annotations (expensive and not favoured approach). Note that using a new namespace URL also indicates that there might be differences in the understanding of the annotations, so the mapping most likely won't be trivial.

All this doesn't mean that we ignore your issue.

We triggered another internal discussion why a different namespace has been chosen for SucessFactors services and whether it wouldn't be better to align with the standard namespace as documented by Gateway (see your own link to the SuccessFactors documentation, which refers to another blog that clearly names the standard namespace).

However, we no longer see this as an issue in the Ui5 client model and that's why we closed the issue here as well as the pull request.

Best Regards, Frank