Task #880
closedCSV connector
Added by Marcel Poul about 7 years ago. Updated over 6 years ago.
100%
Description
1) The aim of this ticket is to implement our own CSV connector, since Tirasa one is %@#$#$^.
It should be really easy and basic. CSV with given format- Includes header
- UTF8
- Every attribute is String
- ';' separator - so it CANNOT be used as column value
- empty value of column is sent to IdM as null
- multi word values in column CAN be inside ""
- no support for multivalued attributes now (be prepare it will be used in the future)
Even though the format is given now, be avare that it is going to be enhanced in the future...
KEEP IT SIMPLE, so every basic administrator of IdM can use it to synchronize data.
2) During connector implementation create a new tutorial to wiki https://wiki.czechidm.com/tutorial
Updated by Marek Klement almost 7 years ago
Technical design:
Backend:
- Connector will implement interfaces IcConnector, (IcCanCreate), IcCanRead, (IcCanDelete), (IcCanUpdate), IcCanGenSchema, IcCanSearch
- Method init() will initialize configuration of this connector
- In configuration will be path to csv, field delimiter, file encoding, key column name, delete column name - mostly basic configuration which cannot be changed
- For parsing CSV will be used CSVParser (com.opencsv.CSVReader)
Frontend:
- Frontend will be same as in Tirasa connector which is available now now
Updated by Radek Tomiška almost 7 years ago
opencsv library is already included in our devstack .)
Updated by Peter Štrunc almost 7 years ago
Connector should not rely on presence of any library and should be shipped as the complete package with dependencies in case anyone else wants to use it outside of CzechIdM. Also use connid archetype (https://github.com/Tirasa/ConnId/tree/master/java/archetype), it will save you a lot of work.
Updated by Radek Tomiška almost 7 years ago
How will be defined dependency in connector? How to prevent libraries version colision?
Updated by Peter Štrunc almost 7 years ago
Same as with all the other connectors that we use. If collision occurs, then user of the library has to solve it for example by excluding the conflicting library in dependency definition in their pom.
Updated by Radek Tomiška almost 7 years ago
So, the safest way is use library included in our devstack and define library as provided in connector (in default profile).
Updated by Peter Štrunc almost 7 years ago
Well if we intend to use it only internally and not ship it as open source bundle, then i guess it is the easier way, but it is common practice not to use provided scope for dependencies, which are necessary for the given artifact to work. In that case, if someone wants to use it in their project, they have to declare dependency not only on csv connector, but also on other libraries which connector needs, but do not provide. But we can do it this way too as long as we document the fact, that the connector needs additional libraries in order to work.
Updated by Radek Tomiška almost 7 years ago
- embedded (or default) - libraries will be provided externally (by devstack)
- standalone (all-jar) - libraries will be included
I prefer the simplest way with possibility to grow.
Updated by Radek Tomiška almost 7 years ago
- Target version deleted (
Garnet (7.7.0))
Updated by Marek Klement over 6 years ago
- Status changed from New to Needs feedback
- Assignee changed from Marek Klement to Vít Švanda
- Target version set to Malachite (9.0.0)
Updated by Vít Švanda over 6 years ago
- Assignee changed from Vít Švanda to Ondřej Kopr
Updated by Petr Michal over 6 years ago
I tested functionality and connector works as required.
Code review is needed.
Updated by Ondřej Kopr over 6 years ago
- Status changed from Needs feedback to In Progress
- Assignee changed from Ondřej Kopr to Marek Klement
- % Done changed from 0 to 50
We checked standard behavior of your connector, there is some mirror bugs:
- if connector doesn't support provisioning, please throw exception. Information in documentation isn't sufficient,
- made synchronization from file that must be ordered by some column is really uncomfortable (please implement synchronization by token and some filter, or allow only reconciliation - discuss this with some good implement guy),
- i just little bit read some code form your connector and I found lines where you throw IllegalArgumentException, this isn't nice, try throw some better exception eq. ConnectorException.
If you want help with implementetaion feel free to discuss with me.
EDIT: please create some future repository in our ghithub for final version of your connector
Updated by Marek Klement over 6 years ago
- Status changed from In Progress to Needs feedback
- Assignee changed from Marek Klement to Ondřej Kopr
- % Done changed from 50 to 60
I solved some problems in the connector:
- as mentioned, the connector doesn't support provisioning, so an exception is thrown if someone tries to proceed it
- synchronization is now disabled too. Is used similarly as in provisioning - so just reconciliation is working.
- all exceptions were replaced by ConnectorException - except ConfigurationExceptions
Can you please do [[https://git.bcvsolutions.eu/modules/csv-connector|code]] review for me.
Thanks
Updated by Marek Klement over 6 years ago
Marek Klement wrote:
I solved some problems in the connector:
- as mentioned, the connector doesn't support provisioning, so an exception is thrown if someone tries to proceed it
- synchronization is now disabled too. Is used similarly as in provisioning - so just reconciliation is working.
- all exceptions were replaced by ConnectorException - except ConfigurationExceptions
Can you please do code review for me - https://git.bcvsolutions.eu/modules/csv-connector
Thanks
Updated by Ondřej Kopr over 6 years ago
- Status changed from Needs feedback to In Progress
- Assignee changed from Ondřej Kopr to Marek Klement
I made first review:
- for your class missing autor, be proud for your code (this connector will be part of public github), also please unite your autor name,
- your connector has silent exception. It is hardly needed throw this exception back to caller (maybe some wrap). Example:
} catch (IOException e) { e.printStackTrace(); }
- check unused imports,
- your log errors are composed with string + string. LOG supports {} for example:
// old LOG.info("ATTRIBUTE "+attribute.getName()+" WITH value "+attribute.getValue()+ " compare to ATTRIBUTE "+attributeValue); // better LOG.info("ATTRIBUTE [{}] WITH value [{}] compare to ATTRIBUTE [{}]", attribute.getName(), attribute.getValue(), attributeValue);
- format your code,
- im not able read some attributes names for example: ret and bh, try apply clean code, another example:
// this is realy ugly AttributeBuilder abID = new AttributeBuilder(); AttributeBuilder abF = new AttributeBuilder(); AttributeBuilder abL = new AttributeBuilder(); AttributeBuilder abG = new AttributeBuilder(); AttributeBuilder abGg = new AttributeBuilder();
- change IllegalArgumentException to connector exception,
- please add some javadoc, even for private methods, you behavior with iterator is little bit hard to read (or use forEach, stream?),
- you didn't close csv reader! while is throw some error reader is still open, please use final statement and close it in final,
- tests for connector didn't pass,
- bad package, for every new connector must be package eu.bcvsolutions.idm.connectors.
Please fix this minor issues, then I will be continue with review.
Updated by Marek Klement over 6 years ago
- Assignee changed from Marek Klement to Ondřej Kopr
Packages do not work properly. Cannot find messages for the connector. Can you please check it Ondro?
Updated by Peter Štrunc over 6 years ago
You need to put messages into the same package Connector class is placed. You refactored the code into a new package so it is probably caused by this.
Updated by Ondřej Kopr over 6 years ago
- Assignee changed from Ondřej Kopr to Marek Klement
I fixed package name and version, then I try build and run your connector, this change works correctly. Please continue with feedback.
There is required steps:- clean your project, remove target folder,
- update correctly your property eq: ic.localconnector.packages=eu.bcvsolutions.idm.connectors.csv.
Updated by Marek Klement over 6 years ago
- Status changed from In Progress to Needs feedback
- Assignee changed from Marek Klement to Ondřej Kopr
- % Done changed from 60 to 70
- Added author to each class and unified name - should be everywhere
- Get rid of silent exception like this:
} catch (IOException e) { throw new ConnectorException(e); }
- checked unused imports - default funcion of IDEA - hopefuly non left
- changed log messages as recommended
- formatted code,
- changed ugly attributes names - every attribute name now seems ok
- couldn't find any other IllegalArgumentException left - tried to find in the project and by looking
- added javadoc
- closed all readers - please check if this works
- tests now work - could not resolve path after a change of directory. Also ignored tests for unsupported methods (exceptions are thrown)
- package is changed to eu.bcvsolutions.idm.connectors
Updated by Ondřej Kopr over 6 years ago
- Status changed from Needs feedback to In Progress
- Assignee changed from Ondřej Kopr to Marek Klement
- % Done changed from 70 to 90
Nice, now your connector works. You can release first version of your connector and publish your connector to public nexus and github.
Updated by Ondřej Kopr over 6 years ago
- Status changed from In Progress to Closed
- % Done changed from 90 to 100
CSV connector was released, there is public git repository: https://github.com/bcvsolutions/csv-connector
nexus public repository: http://nexus.bcvsolutions.eu/#browse/browse/components:maven-public-releases:c60bc6b9612f47d3eefc416e9cf4fd57
maven dependency:
<dependency> <groupId>eu.bcvsolutions.idm.connectors.csv</groupId> <artifactId>csv-connector</artifactId> <version>1.0.0</version> </dependency>
To product was create new pull request: https://github.com/bcvsolutions/CzechIdMng/pull/38
Now can be the csv connector use by another project. Good job.