Skip to content

Commit

Permalink
Versionless feature changes -more review comments
Browse files Browse the repository at this point in the history 
Signed-off-by: Arun Venmany <Arun.Kumar.V.N@ibm.com>
  • Loading branch information
@arunvenmany-ibm
arunvenmany-ibm committed Aug 27, 2024
1 parent f7ef1dd commit ed44828
Show file tree
Hide file tree
Showing 8 changed files with 69 additions and 47 deletions.
12 changes: 4 additions & 8 deletions ...y/src/main/java/io/openliberty/tools/langserver/lemminx/LibertyCompletionParticipant.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,18 +120,14 @@ private void buildPlatformCompletionItems(ICompletionRequest request, ICompletio
request.getXMLDocument().getDocumentURI());
platforms.stream().filter(it->(Objects.isNull(platformName) || it.contains(platformName)))
.filter(p -> !existingPlatforms.contains(LibertyUtils.stripVersion(p).toLowerCase()))
.forEach(item -> {
.forEach(platformItem -> {
Range range = XMLPositionUtility.createRange(parentElement.getStartTagCloseOffset() + 1,
parentElement.getEndTagOpenOffset(), request.getXMLDocument());
Either<TextEdit, InsertReplaceEdit> edit = Either.forLeft(new TextEdit(range, item));
Either<TextEdit, InsertReplaceEdit> edit = Either.forLeft(new TextEdit(range, platformItem));
CompletionItem completionItem = new CompletionItem();
completionItem.setLabel(item);
completionItem.setLabel(platformItem);
completionItem.setTextEdit(edit);
String platformNameNoVersion = LibertyUtils.stripVersion(item).toLowerCase();
if (LibertyConstants.platformDescriptionMap.containsKey(platformNameNoVersion)) {
String version = LibertyUtils.getVersion(item);
completionItem.setDocumentation(String.format(LibertyConstants.platformDescriptionMap.get(platformNameNoVersion),version));
}
completionItem.setDocumentation(LibertyUtils.getPlatformDescription(platformItem));
response.addCompletionItem(completionItem);
});
}
Expand Down
31 changes: 13 additions & 18 deletions ...y/src/main/java/io/openliberty/tools/langserver/lemminx/LibertyDiagnosticParticipant.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,7 @@ private void validateVersionLessFeatures(DOMDocument domDocument, List<Diagnosti
if (versionedFeatures.isEmpty() && preferredPlatforms.isEmpty()) {
Range range = XMLPositionUtility.createRange(versionLessFeatureTextNode.getStart(),
versionLessFeatureTextNode.getEnd(), domDocument);
String message = "ERROR: Need to specify any platform or at least one versioned feature.";
String message = "ERROR: The " + featureName + " versionless feature cannot be resolved. Specify a platform or a feature with a version to enable resolution.";
list.add(new Diagnostic(range, message, DiagnosticSeverity.Error, LIBERTY_LEMMINX_SOURCE, INCORRECT_FEATURE_CODE));
} else if (!versionedFeatures.isEmpty() && preferredPlatforms.isEmpty()) {
Set<String> commonPlatforms = FeatureService.getInstance()
Expand All @@ -475,38 +475,33 @@ else if(commonPlatforms.size()>1) {
list.add(new Diagnostic(range, message, DiagnosticSeverity.Error, LIBERTY_LEMMINX_SOURCE, INCORRECT_FEATURE_CODE));
}
else{
checkForVersionlessPlatforms(domDocument, list, commonPlatforms, versionLessFeatureTextNode, featureName);
checkForVersionlessPlatforms(domDocument, list, commonPlatforms, versionLessFeatureTextNode, featureName, false);
}
}
if (!preferredPlatforms.isEmpty()) {
checkForVersionlessPlatforms(domDocument, list, preferredPlatforms, versionLessFeatureTextNode, featureName);
checkForVersionlessPlatforms(domDocument, list, preferredPlatforms, versionLessFeatureTextNode, featureName,true);
}
}

/**
* check all platforms for versionless features
* @param domDocument
* @param list
* @param preferredPlatforms
* @param versionLessFeatureTextNode
* @param featureName
* @param libertyVersion
* @param libertyRuntime
* @param requestDelay
*/

private void checkForVersionlessPlatforms(DOMDocument domDocument,
List<Diagnostic> list, Set<String> preferredPlatforms,
DOMNode versionLessFeatureTextNode, String featureName) {
List<Diagnostic> list, Set<String> platformsToCompare,
DOMNode versionLessFeatureTextNode, String featureName, boolean isPlatformFromXml) {
LibertyRuntime runtimeInfo = LibertyUtils.getLibertyRuntimeInfo(domDocument);
String libertyVersion = runtimeInfo == null ? null : runtimeInfo.getRuntimeVersion();
String libertyRuntime = runtimeInfo == null ? null : runtimeInfo.getRuntimeType();
final int requestDelay = SettingsService.getInstance().getRequestDelay();
Set<String> allPlatforrmsForVersionLess = FeatureService.getInstance()
.getAllPlatformsForVersionLessFeature(featureName, libertyVersion, libertyRuntime, requestDelay, domDocument.getDocumentURI());
if (Sets.intersection(allPlatforrmsForVersionLess, preferredPlatforms).isEmpty() ){
if (Sets.intersection(allPlatforrmsForVersionLess, platformsToCompare).isEmpty()) {
String message;
if (isPlatformFromXml) {
message = "ERROR: The \"" + featureName + "\" versionless feature does not have a configured platform.";
} else {
message = "ERROR: The \"" + featureName + "\" versionless feature configured platforms cannot be resolved with selected versioned features. Specify a platform or a feature with a version to enable resolution.";
}
Range range = XMLPositionUtility.createRange(versionLessFeatureTextNode.getStart(), versionLessFeatureTextNode.getEnd(),
domDocument);
String message = "ERROR: The \"" + featureName + "\" versionless feature cannot be resolved. Specify a platform or a feature with a version to enable resolution.";
list.add(new Diagnostic(range, message, DiagnosticSeverity.Error, LIBERTY_LEMMINX_SOURCE, INCORRECT_FEATURE_CODE));
}
}
Expand Down
11 changes: 4 additions & 7 deletions ...iberty/src/main/java/io/openliberty/tools/langserver/lemminx/LibertyHoverParticipant.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,17 +80,14 @@ public Hover onText(IHoverRequest request, CancelChecker cancelChecker) {
/**
* get description for platform from the feature json
* there will be a feature with same shortname as platform.
*
* @param platformName platform name
* @return hover
*/
private Hover getHoverPlatformDescription(String platformName) {
String platformNameLowerCase = platformName.toLowerCase();
// strip off version number after the - so that we can provide all possible valid versions of a platform for completion
String platformNameToCompare = platformNameLowerCase.contains("-") ? platformNameLowerCase.substring(0, platformNameLowerCase.lastIndexOf("-")) : platformNameLowerCase;
String version = platformNameLowerCase.contains("-") ? platformNameLowerCase.substring(platformNameLowerCase.lastIndexOf("-") + 1) : "";
if (LibertyConstants.platformDescriptionMap.containsKey(platformNameToCompare)) {
return new Hover(new MarkupContent("plaintext",
String.format(LibertyConstants.platformDescriptionMap.get(platformNameToCompare), version)));
String description = LibertyUtils.getPlatformDescription(platformName);
if (description != null) {
return new Hover(new MarkupContent("plaintext", description));
}
return null;
}
Expand Down
7 changes: 4 additions & 3 deletions ...rty/src/main/java/io/openliberty/tools/langserver/lemminx/codeactions/ReplaceFeature.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,10 @@ public void doCodeAction(ICodeActionRequest request, List<CodeAction> codeAction
Collections.sort(replacementFeatureNames); // sort these so they appear in alphabetical order in quick fixes - also helps the test case pass reliably

for (String nextFeature : replacementFeatureNames) {
String title = "Replace feature with "+nextFeature;

codeActions.add(CodeActionFactory.replace(title, diagnostic.getRange(), nextFeature, document.getTextDocument(), diagnostic));
if (!nextFeature.equals(featureNameToReplace)) {
String title = "Replace feature with " + nextFeature;
codeActions.add(CodeActionFactory.replace(title, diagnostic.getRange(), nextFeature, document.getTextDocument(), diagnostic));
}
}
}
} catch (Exception e) {
Expand Down
4 changes: 2 additions & 2 deletions ...iberty/src/main/java/io/openliberty/tools/langserver/lemminx/services/FeatureService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ public List<Feature> getFeatureReplacements(String featureName, DOMNode featureM
if (existingFeatures.contains(nextFeatureName)) {
// collect feature name minus version number to know which other features to exclude
String featureNameMinusVersion = nextFeatureName.contains("-") ?
nextFeatureName.substring(0, nextFeatureName.lastIndexOf("-") + 1) : nextFeatureName;
nextFeatureName.substring(0, nextFeatureName.lastIndexOf("-") + 1) : nextFeatureName + "-";
featuresWithoutVersionsToExclude.add(featureNameMinusVersion);
}
}
Expand All @@ -283,7 +283,7 @@ public List<Feature> getFeatureReplacements(String featureName, DOMNode featureM
String nextFeatureName = featureNamesLowerCase.get(i);
if (nextFeatureName.contains(featureNameLowerCase)) {
String comparingFeatureName = nextFeatureName.contains("-") ?
nextFeatureName.substring(0, nextFeatureName.lastIndexOf("-") + 1) : nextFeatureName+"-";
nextFeatureName.substring(0, nextFeatureName.lastIndexOf("-") + 1) : nextFeatureName + "-";
if (!featuresWithoutVersionsToExclude.contains(comparingFeatureName)) {
replacementFeatures.add(features.get(i));
}
Expand Down
10 changes: 4 additions & 6 deletions ...-liberty/src/main/java/io/openliberty/tools/langserver/lemminx/util/LibertyConstants.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,12 +48,11 @@ private LibertyConstants() {
}});

public static final Map<String, String> changedFeatureNameMap = Collections.unmodifiableMap(new HashMap<>() {{
put("jsp-", "pages-");
put("jaspic-", "appAuthentication-");
put("jacc-", "appAuthorization-");
put("jca-", "connectors-");
put("jcaInboundSecurity-", "connectorsInboundSecurity-");
put("ejb-", "enterprisebeans-");
put("ejb-", "enterpriseBeans-");
put("ejbHome-", "enterpriseBeansHome-");
put("ejbLite-", "enterpriseBeansLite-");
put("ejbPersistentTimer-", "enterpriseBeansPersistentTimer-");
Expand All @@ -62,15 +61,14 @@ private LibertyConstants() {
put("jsf-", "faces-");
put("javaMail-", "mail-");
put("jms-", "messaging-");
put("jpa-", "enterprisebeans-");
put("ejb-", "persistence-");
put("jpa-", "persistence-");
put("jaxrs-", "restfulWS-");
put("jaxrsClient-", "restfulWSClient-");
put("jsp-", "pages-");
put("jaxb-", "xmlBinding-");
put("jaxws-", "xmlWS-");
put("wasJmsServer-", "messagingServer-");
put("wasJmsClient-", "enterprisebeans-");
put("ejb-", "messagingClient-");
put("wasJmsClient-", "messagingClient-");
put("wasJmsSecurity-", "messagingSecurity-");
}});
}
16 changes: 16 additions & 0 deletions lemminx-liberty/src/main/java/io/openliberty/tools/langserver/lemminx/util/LibertyUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import io.openliberty.tools.langserver.lemminx.services.LibertyProjectsManager;
import io.openliberty.tools.langserver.lemminx.services.LibertyWorkspace;
import io.openliberty.tools.langserver.lemminx.services.SettingsService;
import org.eclipse.lsp4j.CompletionItem;

public class LibertyUtils {

Expand Down Expand Up @@ -558,4 +559,19 @@ public static String stripVersion(String stringWithVersion) {
return stringWithVersion.contains("-") ? stringWithVersion.substring(0, stringWithVersion.lastIndexOf("-"))
: stringWithVersion;
}


/**
* get platform description
* @param platformItem platform item
* @return description
*/
public static String getPlatformDescription(String platformItem) {
String platformNameNoVersion = LibertyUtils.stripVersion(platformItem).toLowerCase();
if (LibertyConstants.platformDescriptionMap.containsKey(platformNameNoVersion)) {
String version = getVersion(platformItem);
return String.format(LibertyConstants.platformDescriptionMap.get(platformNameNoVersion), version);
}
return null;
}
}
25 changes: 22 additions & 3 deletions lemminx-liberty/src/test/java/io/openliberty/LibertyDiagnosticTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -478,7 +478,7 @@ public void testInvalidPlatformForVersionlessFeatureDiagnostic() throws BadLocat
Diagnostic invalid1 = new Diagnostic();
invalid1.setRange(r(2, 24, 2, 31));
invalid1.setCode(LibertyDiagnosticParticipant.INCORRECT_FEATURE_CODE);
invalid1.setMessage("ERROR: Need to specify any platform or at least one versioned feature.");
invalid1.setMessage("ERROR: The servlet versionless feature cannot be resolved. Specify a platform or a feature with a version to enable resolution.");

XMLAssert.testDiagnosticsFor(serverXML, null, null, serverXMLURI,
invalid1);
Expand Down Expand Up @@ -546,7 +546,7 @@ public void testUnresolvedPlatformForVersionlessFeatureDiagnostic() throws BadLo
Diagnostic invalid1 = new Diagnostic();
invalid1.setRange(r(2, 24, 2, 31));
invalid1.setCode(LibertyDiagnosticParticipant.INCORRECT_FEATURE_CODE);
invalid1.setMessage("ERROR: The \"servlet\" versionless feature cannot be resolved. Specify a platform or a feature with a version to enable resolution.");
invalid1.setMessage("ERROR: The \"servlet\" versionless feature configured platforms cannot be resolved with selected versioned features. Specify a platform or a feature with a version to enable resolution.");

XMLAssert.testDiagnosticsFor(serverXML, null, null, serverXMLURI,
invalid1);
Expand All @@ -563,7 +563,7 @@ public void testUnresolvedPlatformForVersionlessFeatureDiagnostic() throws BadLo
invalid1 = new Diagnostic();
invalid1.setRange(r(2, 24, 2, 27));
invalid1.setCode(LibertyDiagnosticParticipant.INCORRECT_FEATURE_CODE);
invalid1.setMessage("ERROR: The \"jsp\" versionless feature cannot be resolved. Specify a platform or a feature with a version to enable resolution.");
invalid1.setMessage("ERROR: The \"jsp\" versionless feature does not have a configured platform.");

XMLAssert.testDiagnosticsFor(serverXML, null, null, serverXMLURI,
invalid1);
Expand All @@ -581,6 +581,25 @@ public void testUnresolvedPlatformForVersionlessFeatureDiagnostic() throws BadLo
invalid1.setCode(LibertyDiagnosticParticipant.INCORRECT_FEATURE_CODE);
invalid1.setMessage("ERROR: The \"servlet\" versionless feature cannot be resolved since there are more than one common platform. Specify a platform or a feature with a version to enable resolution.");

XMLAssert.testDiagnosticsFor(serverXML, null, null, serverXMLURI,
invalid1);

// mpConfig-3.0 and mpJwt-2.0 have a single common platform(microProfile-5.0) but servlet feature does not support that
serverXML = String.join(newLine, //
"<server description=\"Sample Liberty server\">", //
" <featureManager>", //
" <feature>servlet</feature>", //
" <feature>mpConfig-3.0</feature>", //
" <feature>mpJwt-2.0</feature>", //
" </featureManager>", //
"</server>" //
);

invalid1 = new Diagnostic();
invalid1.setRange(r(2, 24, 2, 31));
invalid1.setCode(LibertyDiagnosticParticipant.INCORRECT_FEATURE_CODE);
invalid1.setMessage("ERROR: The \"servlet\" versionless feature configured platforms cannot be resolved with selected versioned features. Specify a platform or a feature with a version to enable resolution.");

XMLAssert.testDiagnosticsFor(serverXML, null, null, serverXMLURI,
invalid1);
}
Expand Down

0 comments on commit ed44828

Please sign in to comment.