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

Implement MS Graph SDK #8103

Merged
merged 29 commits into from
Dec 19, 2024
Merged

Implement MS Graph SDK #8103

merged 29 commits into from
Dec 19, 2024

Conversation

nielsm5
Copy link
Member

@nielsm5 nielsm5 commented Dec 16, 2024

No description provided.

@nielsm5 nielsm5 marked this pull request as ready for review December 16, 2024 12:59
@nielsm5 nielsm5 requested review from evandongen and tnleeuw and removed request for evandongen December 17, 2024 14:57
public static final FileSystemAction[] ACTIONS_BASIC= {FileSystemAction.CREATE, FileSystemAction.LIST, FileSystemAction.INFO, FileSystemAction.READ, FileSystemAction.DOWNLOAD, FileSystemAction.READDELETE, FileSystemAction.MOVE, FileSystemAction.COPY, FileSystemAction.DELETE, FileSystemAction.MKDIR, FileSystemAction.RMDIR};
public static final FileSystemAction[] ACTIONS_WRITABLE_FS= {FileSystemAction.WRITE, FileSystemAction.UPLOAD, FileSystemAction.APPEND, FileSystemAction.RENAME};
public static final FileSystemAction[] ACTIONS_BASIC= {FileSystemAction.LIST, FileSystemAction.INFO, FileSystemAction.READ, FileSystemAction.DOWNLOAD, FileSystemAction.READDELETE, FileSystemAction.MOVE, FileSystemAction.COPY, FileSystemAction.DELETE, FileSystemAction.MKDIR, FileSystemAction.RMDIR};
public static final FileSystemAction[] ACTIONS_WRITABLE_FS= {FileSystemAction.CREATE, FileSystemAction.WRITE, FileSystemAction.UPLOAD, FileSystemAction.APPEND, FileSystemAction.RENAME};
Copy link
Member Author

Choose a reason for hiding this comment

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

create was part of the basic filesystem?

@@ -77,7 +71,7 @@ public void configure() throws ConfigurationException {
attachmentXml.addAttribute("contentType", withAttachments.getAttachmentContentType(attachment));
attachmentXml.addAttribute("size", withAttachments.getAttachmentSize(attachment));
attachmentXml.addAttribute("filename", withAttachments.getAttachmentFileName(attachment));

/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Kan dit eruit?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nee nog liever niet.

}

/** Silly wrapper to create a clean SDK */
public class GraphClient {
Copy link
Contributor

Choose a reason for hiding this comment

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

Kan static zijn

}
}

private static URI validateNextLink(String nextLink) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicate van org.frankframework.filesystem.exchange.MailFolderResponse#validateNextLink - kan dit evt in ExchangeFileSystem?

Copy link
Contributor

@tnleeuw tnleeuw left a comment

Choose a reason for hiding this comment

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

Sonar is boos; behalve de lage coverage heeft Sonar ook nog heel wat opmerkingen over kleine dingen die op zich wel terecht lijken.
Zou je daar misschien nog wat aan kunnen doen?

folders.addAll(response.folders);

if (StringUtils.isNotBlank(response.nextLink) && limit > 0) {
getRecursive(client, validateNextLink(response.nextLink), folders, limit - MAX_ENTRIES_PER_CALL);
Copy link
Contributor

Choose a reason for hiding this comment

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

Je had het vorige week over tail-recursion, maar dit is niet echte tail-recursion... 😬

Copy link
Member Author

Choose a reason for hiding this comment

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

Dat is toch het doel? Niet genest recursie hebben?

Comment on lines 178 to 184
List<MailFolder> folders = client.getMailFolders(mailAddress);
MailFolder foundMailFolder = folders.stream()
.filter(t -> rootFolder.equalsIgnoreCase(t.getName()))
.findFirst()
.orElseThrow(() -> {
throw new LifecycleException("unable to find folder [%s] in mailbox [%s]".formatted(rootFolder, mailAddress));
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Je haalt hiermee max 20 folders op. Als een mailbox meer dan 20 folders heeft, en de folder die je zoekt zit niet in de eerste 20, dan vind je die dus nooit en kan je nooit het FS openen -- ondanks dat de folder eigenlijk wel bestaat.

Wil je aan dit probleem nog iets doen in dit PR? Of in een volgend PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

20 per request, en 20 requests :)

Copy link
Contributor

Choose a reason for hiding this comment

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

In de orElseThrow hoor je trouwens niet zelf te throwen, alleen de new Exception terug te geven.

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
8.0% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@nielsm5 nielsm5 linked an issue Dec 19, 2024 that may be closed by this pull request
@nielsm5 nielsm5 merged commit 2c7a345 into master Dec 19, 2024
22 of 24 checks passed
@nielsm5 nielsm5 deleted the implement-ms-graph-sdk branch December 19, 2024 12:41
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.

Sending e-mails using MS Graph Entra Id
3 participants