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

feat(elation): rate limiting with custom rate limiter #582

Merged
merged 2 commits into from
Feb 6, 2025

Conversation

bejoinka
Copy link
Contributor

@bejoinka bejoinka commented Feb 6, 2025

  • feat(elation): add rate limiter to elation webhooks
  • feat(elation): improved rate limiting settings validation and logging

Details

The PR enhances the rate limit duration validation by:

  1. Supporting multiple input formats for time units:
    • Seconds: 's', 'second', 'seconds'
    • Minutes: 'm', 'min', 'minute', 'minutes'
    • Hours: 'h', 'hour', 'hours'
    • Days: 'd', 'day', 'days'
  2. Maintaining backward compatibility with existing settings
  3. Providing clear error messages for invalid inputs
  4. Preserving optional setting functionality

Testing

  • Added comprehensive test cases covering:
    • Valid formats with different units
    • Edge cases (undefined values)
    • Invalid formats
    • Unit variations

Copy link

github-actions bot commented Feb 6, 2025

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Validation Logic

The rateLimitDurationSchema introduces custom validation and transformation logic. Ensure this logic is robust, handles edge cases, and aligns with expected input formats.

export const rateLimitDurationSchema = z
  .string()
  .refine(
    (val) => {
      try {
        const [number, unit] = val.split(' ')
        const parsedUnit = parseDurationUnit(unit)
        return isFinite(Number(number)) && !isNil(parsedUnit)
      } catch (error) {
        return false
      }
    },
    {
      message:
        'Duration must be in format {number} {unit} where unit is seconds, minutes, hours or days',
    },
  )
  .transform((val): RateLimitConfig['duration'] => {
    const [number, unit] = val.split(' ')
    const parsedUnit = parseDurationUnit(unit)
    return {
      value: Number(number),
      unit: parsedUnit,
    }
  })
  .optional()

export const SettingsValidationSchema = z.object({
  base_url: z.string().min(1),
  auth_url: z.string().min(1),
  client_id: z.string().min(1),
  client_secret: z.string().min(1),
  /**
   * Elation now uses client credentials authentication.
   * We don't remove the settings just yet for backward compatibility for existing care flows.
   * See https://linear.app/awell/issue/ET-577/elation-extension-make-username-and-password-optional-in-auth
   */
  username: z.string().optional(),
  password: z.string().optional(),
  rateLimitDuration: rateLimitDurationSchema,
} satisfies Record<keyof typeof settings, ZodTypeAny>)

export type SettingsType = z.infer<typeof SettingsValidationSchema>

const parseDurationUnit = (
  unit: string | undefined,
): 'seconds' | 'minutes' | 'hours' | 'days' => {
  if (!unit) throw new Error('Duration unit is required')

  const normalized = unit.toLowerCase().trim()

  switch (normalized) {
    case 's':
    case 'second':
    case 'seconds':
      return 'seconds'

    case 'm':
    case 'min':
    case 'minute':
    case 'minutes':
      return 'minutes'

    case 'h':
    case 'hour':
    case 'hours':
      return 'hours'

    case 'd':
    case 'day':
    case 'days':
      return 'days'

    default:
      throw new Error(
        `Invalid duration unit: ${unit}. Valid units are: s, m, h, d`,
      )
  }
}
Rate Limiting Integration

The onEvent function now includes rate limiting logic. Validate that the implementation correctly handles rate-limited requests and logs appropriate warnings or errors.

onEvent: async ({
  payload: { payload, settings },
  onSuccess,
  onError,
  helpers: { rateLimiter },
}) => {
  const { action, resource, data } = payload
  const { id: appointmentId, patient: patientId } = data

  // skip non 'saved'  actions for that webhook
  if (action !== 'saved') {
    return
  }

  // rate limiting
  const { success, data: duration } = rateLimitDurationSchema.safeParse(
    settings.rateLimitDuration,
  )
  if (success === true && !isNil(duration)) {
    const limiter = rateLimiter('elation-appointment', {
      requests: 1,
      duration,
    })
    const { success } = await limiter.limit(appointmentId.toString())
    if (!success) {
      console.warn({
        data,
        resource,
        action,
        message:
          'Rate limit exceeded. 200 OK response sent to Elation to prevent further requests.',
      })
      await onError({
        response: {
          statusCode: 200,
          message:
            'Rate limit exceeded. 200 OK response sent to Elation to prevent further requests.',
        },
      })
      return
    }
    console.log(`Rate limit success for appointment_id=${appointmentId}`)
  }
Rate Limiting Integration

Similar to appointmentCreatedOrUpdated, ensure the rate limiting logic in patientCreatedOrUpdated is correctly implemented and handles edge cases effectively.

onEvent: async ({
  payload: { payload, settings },
  onSuccess,
  onError,
  helpers: { rateLimiter },
}) => {
  const { data, resource, action } = payload
  const { id: patientId } = data
  // skip non 'saved' actions for that webhook
  if (action !== 'saved') {
    return
  }

  // rate limiting
  const { success, data: duration } = rateLimitDurationSchema.safeParse(
    settings.rateLimitDuration,
  )
  if (success === true && !isNil(duration)) {
    const limiter = rateLimiter('elation-patient', {
      requests: 1,
      duration,
    })
    const { success } = await limiter.limit(patientId.toString())
    if (!success) {
      console.warn({
        data,
        resource,
        action,
        message:
          'Rate limit exceeded. 200 OK response sent to Elation to prevent further requests.',
      })
      await onError({
        response: {
          statusCode: 200,
          message:
            'Rate limit exceeded. 200 OK response sent to Elation to prevent further requests.',
        },
      })
      return
    }
    console.log(`Rate limit success for patient_id=${patientId}`)

Copy link

github-actions bot commented Feb 6, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Handle rate limiter errors gracefully

Add error handling for the rateLimiter.limit call to manage unexpected errors and
ensure the application does not crash.

extensions/elation/webhooks/appointmentCreatedOrUpdated.ts [50]

-const { success } = await limiter.limit(appointmentId.toString())
+try {
+  const { success } = await limiter.limit(appointmentId.toString());
+} catch (error) {
+  console.error('Rate limiter error:', error);
+  await onError({ response: { statusCode: 500, message: 'Internal server error' } });
+  return;
+}
Suggestion importance[1-10]: 10

__

Why: Wrapping the rateLimiter.limit call in a try-catch block is essential to handle unexpected errors, ensuring the application does not crash and providing a meaningful error response.

High
Add error handling for rate limiter

Ensure that the rateLimiter.limit call is wrapped in a try-catch block to handle
potential runtime errors and avoid unhandled exceptions.

extensions/elation/webhooks/patientCreatedOrUpdated.ts [48]

-const { success } = await limiter.limit(patientId.toString())
+try {
+  const { success } = await limiter.limit(patientId.toString());
+} catch (error) {
+  console.error('Rate limiter error:', error);
+  await onError({ response: { statusCode: 500, message: 'Internal server error' } });
+  return;
+}
Suggestion importance[1-10]: 10

__

Why: Including error handling for the rateLimiter.limit call is crucial to prevent unhandled exceptions, ensuring the application remains stable and provides appropriate error responses.

High
Validate positive duration numbers

Add validation to ensure that the number part of the duration string is a positive
integer to prevent invalid or negative durations from being processed.

extensions/elation/settings.ts [67]

-return isFinite(Number(number)) && !isNil(parsedUnit)
+return isFinite(Number(number)) && Number(number) > 0 && !isNil(parsedUnit)
Suggestion importance[1-10]: 9

__

Why: Adding validation to ensure the duration number is positive is a critical improvement that prevents invalid or negative durations from being processed, enhancing the robustness of the code.

High
General
Add tests for invalid durations

Include test cases for invalid duration formats like negative numbers or unsupported
units to ensure comprehensive validation coverage.

extensions/elation/webhooks/rateLimitValidation.test.ts [27]

-it.each(['1s', '6m', '12h', '86400m', '30d', 'invalid', ''])(
+it.each(['1s', '6m', '12h', '86400m', '30d', 'invalid', '', '-5 s', '10 weeks'])(
Suggestion importance[1-10]: 8

__

Why: Adding test cases for invalid duration formats improves the test coverage and ensures the validation logic is robust against edge cases, such as negative numbers or unsupported units.

Medium

@bejoinka bejoinka merged commit 1802dab into main Feb 6, 2025
2 checks passed
@bejoinka bejoinka deleted the et-913-implement-rate-limiter-in-extensions branch February 6, 2025 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant