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

Implements InMemory AASX file server #84

Merged
merged 20 commits into from
Nov 8, 2023

Conversation

KBChaithra
Copy link
Contributor

  • Adds core
  • Adds backend-inmemory
  • Adds component

Signed-off-by: Chaithra Kandur Bhagavanthaiah [email protected]

- Adds core
- Adds backend-inmemory
- Adds component

Signed-off-by: Chaithra Kandur Bhagavanthaiah <[email protected]>
Signed-off-by: Chaithra Kandur Bhagavanthaiah <[email protected]>
Signed-off-by: Chaithra Kandur Bhagavanthaiah <[email protected]>
Signed-off-by: Chaithra Kandur Bhagavanthaiah <[email protected]>
Signed-off-by: Chaithra Kandur Bhagavanthaiah <[email protected]>
Copy link
Contributor

@mdanish98 mdanish98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following are the review remarks.

/**
* Retrieves all AASXPackageIds from the repository
*
* @return a list of available AASX packages at the server
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please improve this, as it is not returning the AASX packages.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

public interface AasxFileServer {

/**
* Retrieves all AASXPackageIds from the repository
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AASX package ids

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

* @param packageId
* @throws ElementDoesNotExistException
*/
public void deleteAASXPackageById(String packageId) throws ElementDoesNotExistException;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename it to deleteAASXByPackageId

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

public PackageDescription createAASXPackage(List<String> aasIds, InputStream file, String fileName) throws CollidingIdentifierException;

/**
* Deletes a AASXPackage
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AASX Package

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

public void updateAASXByPackageId(String packageId, List<String> aasIds, InputStream file, String filename) throws ElementDoesNotExistException;

/**
* Creates a new AASXPackage
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AASX Package

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

packageMap.remove(packageId);
}

public Integer increment(Integer n) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No usage of this method, please remove this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

return n++;
}

private PackageDescription createPackageDescription(List<String> aasIds, String newpackageId) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newPackageId

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

return packagesBody;
}

private void createPackage(List<String> aasIds, InputStream file, String fileName, String newpackageId, PackageDescription packageDescription) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

newPackageId

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

public Collection<PackageDescription> getAllAASXPackageIds() {

return packageMap.values().stream()
.map(aasxPackage -> aasxPackage.getPackageDescription())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use Package::getPackageDescription

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

import org.springframework.stereotype.Component;

/**
* AasxFileServer factory returning an in-memory backend AasxFileServer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use @link to mention the class

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

PackageDescription expectedFirstPackage = iterator.next();
PackageDescription expectedSecondPackage = iterator.next();

assertTrue(packageDescriptions.containsAll(Arrays.asList(expectedFirstPackage, expectedSecondPackage)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong, actual is same as the expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes done

Copy link
Contributor

@mdanish98 mdanish98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following are the review remarks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent naming of class.

Please either use AASX* or Aasx* everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent naming of class.

Please either use AASX* or Aasx* everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please put this inside a new package named "model".

Also, put PackDesc and PackBody classes in the same package.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

package org.eclipse.digitaltwin.basyx.aasxfileserver;

/**
* Specifies the Package for AasxFileServer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use @_link to reference classes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

import org.eclipse.digitaltwin.basyx.core.exceptions.ElementDoesNotExistException;

/**
* Specifies the overall AasxFileServer API
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use @_link to reference classes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

* @param filename
* @throws CollidingIdentifierException
*/
public PackageDescription createAASXPackage(List<String> aasIds, InputStream file, String fileName) throws CollidingIdentifierException;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As identifier is generated automatically at runtime, there is no chance of collision.

Please remove this throws declaration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}

@Override
public PackageDescription createAASXPackage(List<String> aasIds, InputStream file, String fileName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As identifier is generated automatically at runtime, there is no chance of collision.

Please remove this throws declaration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

import org.eclipse.digitaltwin.basyx.core.exceptions.ElementDoesNotExistException;

/**
* In-memory implementation of the AASXFileServer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use @_link

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check and add all other missing properties.

Refer other repositories component.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The createAASXPackage test is missing, please add it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added the test case

Copy link
Contributor

@jannisjung jannisjung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please resolve the review remarks

*
* @return a list of available AASX Package Descriptions at the server
*/
public Collection<PackageDescription> getAllAASXPackageIds();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing param aasId (see SPOTAAS Part2, 5.5.2 Operation GetAllAASXPackageIds, p. 45)
e.g. overload method

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(according to our naming guide lines the param should be named shellId)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated this method

protected abstract AASXFileServer getAASXFileServer();

@Test
public void getAllAASXPackageIds() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a test case for getAASXFileServer(String shellId)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a test case.

public AASXFileServerFeaturePrinter(List<AASXFileServerFeature> features) {
logger.info("-------------------- AASX File Server Features: --------------------");
for (AASXFileServerFeature feature : features) {
logger.info("BaSyxFeature " + feature.getName() + " is enabled: " + feature.isEnabled());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logger Strings can be formatted like
logger.info("BaSyxFeature '{}' is enabled: {}", feature.getName(), feature.isEnabled())

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formatted the logger

Signed-off-by: Mohammad Ghazanfar Ali Danish <[email protected]>
@FrankSchnicke FrankSchnicke merged commit 664073b into eclipse-basyx:main Nov 8, 2023
1 check passed
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.

4 participants