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

Fix Dialog URL on ServiceProviderFactory #236

Closed
wants to merge 6 commits into from

Conversation

isccarrasco
Copy link
Contributor

Description

Solves the issue found in the ServiceProviderFactory in which the URL for the Dialogs are assigned to the same URL of the OSL API, the fix is to set the URL to the web application in which the html files are hosted

Checklist

  • This PR adds an entry to the CHANGELOG. See https://keepachangelog.com/en/1.0.0/ for instructions. Minor edits are exempt.
  • This PR was tested on at least one Lyo OSLC server or adds unit/integration tests.
  • This PR does NOT break the API

Issues

Closes #235

(use exactly this syntax to link to issues, one issue per statement only!)

Adding the section to look for the catalogUrl endpoint of the
Configuration Management service from the Rootservices.

Signed-off-by: Mario Jiménez Carrasco <[email protected]>
Use the URI used on the creation of the dialog to use
the correct URL that contains the genericBaseURI which
represents the context.

Close eclipse-lyo#235

Signed-off-by: isccarrasco <[email protected]>
@berezovskyi
Copy link
Contributor

@isccarrasco thank you for the PR! Looks good to me but we need to test it because I forgot the difference between the two arguments and we should be able to merge it soon.

@berezovskyi berezovskyi added this to the 5.0 milestone Feb 12, 2022
@berezovskyi
Copy link
Contributor

berezovskyi commented Feb 12, 2022

Mario, I just took a look at how this code is used now. Sadly, we don't have any test coverage for this. Right now the adaptors during initialization provide the base URI via baseUri and set genericBaseUri to an empty string "". The dialogUri is a relative URI path fragment that is then concatenated with the base URI.

Screen Shot 2022-02-12 at 18 37 09

Screen Shot 2022-02-12 at 18 41 39

Screen Shot 2022-02-12 at 18 46 44

Instead, the genericBaseUri is used to auto-generate the URIs of the dialogs with the dialogUri property. I don't think there is any code known to me that relies on "generic" URIs because they still have to point to existing pages that serve dialogs (and in that case it would not hurt to give a proper dialog URI path fragment).

Screen Shot 2022-02-12 at 19 04 15

How do you hit this problem? Our code calls ServiceProviderFactory.createServiceProvider with a proper baseUri, empty genericBaseUri, and the Dialog annotation looks like this:

@org.eclipse.lyo.oslc4j.core.annotation.OslcDialog(resourceTypes={"http://open-services.net/ns/rm#Requirement"}, usages={}, label="SelectionDialog1", hintWidth="0px", hintHeight="0px", title="SelectionDialog1", uri="requirements/selector")

I am guessing you are passing the full URI to dialogUri? I can totally see how could this be confusing. I think the reason the implementation is done this way is because you cannot use non-const expressions in @Dialog annotations. Maybe we should change the parameter name to relativeUri instead of uri or add a check for URI being absolute and then ignore both the base URI and the "generic" base URI.

Also, how are you using those "generic" base URIs? I am thinking to deprecate URI guessing for dialogs that are not fully annotated altogether to avoid confusion.

CC @jadelkhoury

@jadelkhoury
Copy link
Contributor

Hi

I just tried to walk through the code. I am not sure what this change can solve. Can you please Mario? I can see though how this can be problematic since the genericBaseUri seem to have a whole different purpose. Making the suggested change will certainly change the current behaviour.

as suggested by @berezovskyi , will not passing the baseURI as you expect it to be do the job?

@jadelkhoury
Copy link
Contributor

So, I normally set the baseUri to OSLC4JUtils.getServletURI() to make it refer to the same base as hte rest of the OSLC server. But I cannot see any thing hindering passing any other parameter.

@eclipse-lyo-bot
Copy link

Can one of the admins verify this patch?

@berezovskyi
Copy link
Contributor

add to whitelist

@berezovskyi berezovskyi removed this from the 5.0 milestone May 9, 2022
@berezovskyi
Copy link
Contributor

I removed the 5.0 milestone not to block the release.

@sonarcloud
Copy link

sonarcloud bot commented May 13, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@berezovskyi
Copy link
Contributor

Closing this PR without merging. @isccarrasco if you still have this problem, please reply to #236 (comment) and we will consider reopening it. Thank you!

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

Successfully merging this pull request may close these issues.

Dialog URL uses the API URL
4 participants